Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vars not reported as reassigned/mutated when directly done in template #3793

Closed
Conduitry opened this issue Oct 26, 2019 · 5 comments
Closed
Labels
bug compiler Changes relating to the compiler temp-stale

Comments

@Conduitry
Copy link
Member

<script>
	let foo = 0;
	let bar = 0;
</script>
{() => foo = 1}
{bar = 1}

The vars in the compiler output reports foo as reassigned, but does not report bar as ever reassigned. I don't know what effects this might have on the compiled output, but for the ESLint plugin with prefer-const enabled, it does result in bar being erroneously turned into a const.

@Conduitry Conduitry added the bug label Oct 26, 2019
@tanhauhau
Copy link
Member

tanhauhau commented Oct 28, 2019

It renders correctly with bar = 1

However, if bar is marked as reassigned or mutated due to code somewhere else, it will have a runtime error:

<script>
	let bar = 0;
</script>

<h1>{bar = 1}</h1>
<h2>{bar}</h2>
<button on:click={() => {bar  }}>bar   </button>

@Conduitry
Copy link
Member Author

The runtime error mentioned above is no longer happening as of 3.13.0, but there's another issue - the inline update in the template is only affecting the component's ctx object/array, and not the bar variable that's accessed in the event handlers. Changes made in the event handler are making their way into ctx (by way of $$invalidate) but changes to ctx can't find their way back.

@tanhauhau
Copy link
Member

was revisiting this, and realise that @Rich-Harris wrote a comment on this

if (node.type === 'AssignmentExpression') {
// TODO should this be a warning/error? `<p>{foo = 1}</p>`
}

we can mark it as referenced/mutated to prevent eslint to change it, but still i dont see any reason of wanting to write:

{bar = 1}

and regarding the impact on compiled output, i tried, it seems that the only difference is that, it will try to be reactive in the update function:

   if (dirty & /*bar*/ 4 && t4_value !== (t4_value = (/*bar*/ ctx[2] = 1)   "")) set_data_dev(t4, t4_value);

yet, as you said, the changes made here may / may not make its way back to the ctx, (depending whether we optimistically destruct the ctx array it in the params or not based on the number of variables), still it does not trigger $$invalidate to run the update cycle.

@Conduitry
Copy link
Member Author

Writing something like {bar = 1} has come up a number of times, with people wondering how to do 'inline' assignments to variables, often for one iteration of an each loop. For example:

<script>
	let y;
</script>

{#each [1, 2, 3, 4, 5] as x}
	{(y = x * x, '')}
	<div>
		{y}
	</div>
{/each}

This is something we probably want to discourage, but at this point, depending on how many people are using it, it might not be something I feel comfortable making stop working.

A few months ago, something else I had suggested in chat was to make assignments from the template not trigger reactivity. That would let something like {foo} {(foo = 1, '')} {foo} work in a (possibly) slightly more expected way, where I think the two values shown for foo would differ. This might be a really bad idea, though, I'm not sure.

I suppose on the whole, I'm a little worried about doing something like #4492 and calling it a bugfix, because it seems to start to make a decision about some of this stuff that would be a bit hard to walk back later. I feel like I want to have at least some answers for how all this stuff should work before proceeding, and I don't think I do currently.

@Rich-Harris
Copy link
Member

Closing as this behaves correctly in Svelte 5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug compiler Changes relating to the compiler temp-stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants