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

class directive generates update code for unreactive dependencies #5919

Closed
pushkine opened this issue Jan 22, 2021 · 2 comments · Fixed by #5926
Closed

class directive generates update code for unreactive dependencies #5919

pushkine opened this issue Jan 22, 2021 · 2 comments · Fixed by #5926

Comments

@pushkine
Copy link
Contributor

<script>
	const o = { v: true }
</script>

<div class:test={o.v}>
	{o.v}
</div>
p(ctx, [dirty]) {
	if (dirty & /*o*/ 1) {
		toggle_class(div, "test", /*o*/ ctx[0].v);
	}
}

https://svelte.dev/repl/5b14ec60637b43a184460475790105c0?version=3.31.2

Given that <div>{o.v}</div> does not generate update code, neither should the class directive
Expected noop at fragment.p

@j-delaney
Copy link
Contributor

I've been thinking a lot about this myself!

Because of how JavaScript const works, I think doing this optimization with all consts would be pretty hard. In your example you can change the value of o.v because const only guarantees that o refers to the same object, not that it doesn't change (which is pretty silly, but it's what they chose to do). See https://svelte.dev/repl/b435e75ce57f440b84993eb809eb7575?version=3 for an example. To get around that you would have to do some pretty gnarly code analysis (see if that variable is ever written to, including understanding if it can be passed into a function that writes to it).

I think a simpler, more direct optimization would be to limit this optimization to const primitives (e.g. only const o = true) because in those cases o can't be changed.

@Conduitry
Copy link
Member

As of 3.32.1, the update method is now a no-op https://svelte.dev/repl/5b14ec60637b43a184460475790105c0?version=3.32.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants