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 not working with $$restProps #15386

Closed
metonym opened this issue Feb 25, 2025 · 3 comments · Fixed by #15389
Closed

class:directive not working with $$restProps #15386

metonym opened this issue Feb 25, 2025 · 3 comments · Fixed by #15389

Comments

@metonym
Copy link
Contributor

metonym commented Feb 25, 2025

Describe the bug

I'm experiencing this bug with Svelte versions 5.20.3 and 5.20.4.

If I have a component that uses a class: directive to toggle a class and also spread $$restProps to it, the class: directive is not updated.

<!-- `class:active` is not updated if spreading $$restProps -->
<div class:active={active} {...$$restProps}>

<!-- The markup is correctly updated if `active` changes -->
  <slot /> {active}
</div>

In the provided repro, do the following:

  1. Click the "Toggle" button
  2. The active prop passed to the Component should turn it blue
  3. Click "Toggle" again
  4. The {active} updates in the slot, but the class directive is not updated

Now remove the $$restProps from the Component; it works as expected. The expected behavior is that the class: directive should still re-compute with $$restProps being spread.

From what I can tell, this was working with early patches of 5.20.

Based on the changelog of 5.20.3, I wonder if this could be related to the issue?

fix: correctly override class attributes with class directives

For additional context, I stumbled upon this when investigating this issue for Carbon Svelte: carbon-design-system/carbon-components-svelte#2102

Reproduction

https://svelte.dev/playground/e7cf33de70ae4059a5c99bb3b6c8fefa?version=5.20.4

Logs

System Info

System:
  OS: macOS 13.5.2
  CPU: (8) x64 Intel(R) Core(TM) i5-8257U CPU @ 1.40GHz
  Memory: 185.26 MB / 16.00 GB
  Shell: 5.9 - /bin/zsh
Binaries:
  Node: 20.11.1 - ~/.nvm/versions/node/v20.11.1/bin/node
  npm: 9.9.3 - ~/.nvm/versions/node/v20.11.1/bin/npm
  pnpm: 9.15.0 - ~/Library/pnpm/pnpm
  bun: 1.2.3 - ~/.bun/bin/bun
Browsers:
  Chrome: 133.0.6943.127
  Safari: 16.6
npmPackages:
  svelte: 5.20.4 => 5.20.4

Severity

blocking an upgrade

@adiguba
Copy link
Contributor

adiguba commented Feb 25, 2025

My bads :/

I think I found the problem here :

} else if (key === 'class') {
var is_html = element.namespaceURI === 'http://www.w3.org/1999/xhtml';
set_class(element, is_html, value, css_hash, prev?.[CLASS], next[CLASS]);
} else if (key === 'style' && value != null) {

The directives are not copied to the current state, so falsy value are not correctly updated :(

This should solve the problem :

		} else if (key === 'class') {
			var is_html = element.namespaceURI === 'http://www.w3.org/1999/xhtml';
			set_class(element, is_html, value, css_hash, prev?.[CLASS], next[CLASS]);
+			current[CLASS] = next[CLASS];
		} else if (key === 'style' && value != null) {

And it would be better to do an additional check in set_class()

} else if (next_classes) {
prev_classes ??= {};
for (var key in next_classes) {
var is_present = !!next_classes[key];
if (is_present !== !!prev_classes[key]) {
dom.classList.toggle(key, is_present);
}
}

	} else if (next_classes) {
-		prev_classes ??= {};
+		var force_update = prev_classes == null;

		for (var key in next_classes) {
			var is_present = !!next_classes[key];

			if (force_update  || is_present !== !!prev_classes[key]) {
				dom.classList.toggle(key, is_present);
			}
		}

I cannot make a PR for now, so if anyone can do it...

adiguba added a commit to adiguba/svelte that referenced this issue Feb 25, 2025
@kyoshino
Copy link
Contributor

I’m seeing the same problem with runes mode enabled. Source

@adiguba
Copy link
Contributor

adiguba commented Feb 26, 2025

Yes the bug was introduced in the set_attributes() function that handle spreading both in legacy and runes mode.

It's fixed on the PR in legacy mode or runes mode

trueadm pushed a commit that referenced this issue Feb 26, 2025
* add spread to test

* fix #15386

* do no set cssHash on non-scoped element

* changeset
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.

3 participants