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

fix: ensure event precedence when spreading event attributes #9433

Merged
merged 7 commits into from
Nov 14, 2023

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Nov 13, 2023

Turns out we aren't correctly handling spreading of attributes when there's a delegated event. This was a bit more involved than I'd like as we had to ensure delegated events occurred before the spread, but it makes sense.

Copy link

changeset-bot bot commented Nov 13, 2023

🦋 Changeset detected

Latest commit: 42ce088

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Nov 13, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-5-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 14, 2023 9:20pm

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should fix this differently and add them to the spread array instead like we do when there's a mix of normal and spread attributes. While doing that we could then investigate how to Code-Golf event delegation inside the spread method.

@trueadm
Copy link
Contributor Author

trueadm commented Nov 13, 2023

@dummdidumm Yeah, that's a good call. Would avoid the property lookup which is much nicer. Feel free to take over this PR. I think the bigger issue found is that snippets don't properly hydrate today because we don't serialize anchors in SSR (they look to be completely missing), plus I don't know why snippets are always blocks – they should only be if the snippet is dynamic, just like components work.

@dummdidumm
Copy link
Member

  • added event delegation to spread_attributes
  • add event attributes to the spread when there is one
  • fixed an edge case bug with event hoistability: if you did
<button on:click={increment}>
	clicks: {count}
</button>
<button on:click={increment} on:click={increment}>
	clicks: {count}
</button>

Then the first one would incorrectly determine that increment can be hoisted

  • simplified the test to test only what's important to us here. The hydration thing should be handled separately

…preserve backwards compatibility of ordering
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 this pull request may close these issues.

None yet

2 participants