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.

Sign up for GitHub

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

Jump to bottom

bugfix(svelte): fix colliding id's generated from $props.id #13339

Merged
merged 14 commits into from
Mar 5, 2025
Merged

bugfix(svelte): fix colliding id's generated from $props.id #13339

merged 14 commits into from
Mar 5, 2025
+99 −64

Conversation

Hugos68
Copy link
Contributor

@Hugos68 Hugos68 commented Feb 28, 2025 •
edited
Loading

Closes #13327

Utilizes the new idPrefix option from server/client side rendering added in sveltejs/svelte#15403 in Svelte to inject a unique prefix generated by Astro.

Changes

  • Enables different Svelte islands to generate non colliding $props.id

Testing

No tests were added because it looks like the Svelte integration isn't tested, at all?

Docs

This would only solve problems, it's highly unlikely a user depended on colliding ID's for their app to work. It's also a very niche use case using two client side Astro Svelte components with $props.id

@Hugos68
bugfix(svelte): fix colliding id's generating from $props.id by utili…
Loading
Loading status checks…
dbd918c 
…zing the new `uidPrefix` option exposed for both server and client side rendering.
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Feb 28, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 37bc918

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

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

@github-actions github-actions bot added pkg: svelte Related to Svelte (scope) pkg: integration Related to any renderer integration (scope) labels Feb 28, 2025
@Hugos68
bugfix(svelte): changeset
Loading
Loading status checks…
94b8c20
@florian-lefebvre
Copy link
Member

@Hugos68 ping me when the related svelte PR is merged and released

@Hugos68
Copy link
Contributor Author

Hugos68 commented Mar 4, 2025

@florian-lefebvre Merged and released in: sveltejs/svelte#15428. API is available in version 5.22.0!

@Hugos68
Copy link
Contributor Author

Hugos68 commented Mar 4, 2025

I'll aim to update my PR and will request a review after.

@Hugos68
Copy link
Contributor Author

Hugos68 commented Mar 4, 2025

@florian-lefebvre I've implemented the prefix for the server side part of islands. I don't know how to apply it to the client, some help would be much appreciated.

@florian-lefebvre
Copy link
Member

I'll look into it later today!

@florian-lefebvre
Copy link
Member

Looking at sveltejs/svelte#15428, I think svelte takes care of ids on the client

florian-lefebvre
florian-lefebvre reviewed Mar 5, 2025
View reviewed changes
Hugos68 and others added 3 commits March 5, 2025 11:16
@Hugos68 @florian-lefebvre
Update .changeset/flat-cherries-rule.md
Loading
Loading status checks…
9c032ec 
Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
@florian-lefebvre
Merge branch 'main' into bugfix/svelte-collide-props-id
8243087
@florian-lefebvre
Merge branch 'bugfix/svelte-collide-props-id' of https://github.com/h…
Loading
Loading status checks…
b0b256f 
…ugos68/astro into bugfix/svelte-collide-props-id
@florian-lefebvre
feat: context
Loading
Loading status checks…
bb9a01b
@Hugos68
Copy link
Contributor Author

Hugos68 commented Mar 5, 2025

incrementId came from context, but I see you've added the missing import and file. Like I said, I was a bit puzzled so ended up reverting a bunch of stuff but didn't that correctly.

Hugos68 added 2 commits March 5, 2025 11:24
@Hugos68
revert peerDep bump
Loading
Loading status checks…
e608f7e
@Hugos68
revert html variable seperation
Loading
Loading status checks…
a186e07
@Hugos68
Copy link
Contributor Author

Hugos68 commented Mar 5, 2025

Looking at sveltejs/svelte#15428, I think svelte takes care of ids on the client

Ok, sounds good, so then if the server side prefixing looks correct I'll move it to a ready for review PR.

@Hugos68 Hugos68 marked this pull request as ready for review March 5, 2025 10:27
florian-lefebvre
florian-lefebvre approved these changes Mar 5, 2025
View reviewed changes
Copy link
Member

@florian-lefebvre florian-lefebvre left a comment

Choose a reason for hiding this comment

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

Thanks for the help and the contribution upstream!

@florian-lefebvre florian-lefebvre self-assigned this Mar 5, 2025
@Hugos68
Copy link
Contributor Author

Hugos68 commented Mar 5, 2025

Yeah I think I borked something when merging upstream changes back to this branch, thanks for picking these up.

@florian-lefebvre florian-lefebvre merged commit a05e6ab into withastro:main Mar 5, 2025
26 of 27 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Mar 5, 2025
Merged
hkbertoson pushed a commit to hkbertoson/astro that referenced this pull request Mar 6, 2025
@Hugos68 @florian-lefebvre @hkbertoson
bugfix(svelte): fix colliding id's generated from $props.id (withas…
e33bb79 
…tro#13339)

* bugfix(svelte): fix colliding id's generating from $props.id by utilizing the new `uidPrefix` option exposed for both server and client side rendering.

* bugfix(svelte): changeset

* Add server side ID prefix, not sure how to do this on the client.

* Discard changes to packages/integrations/svelte/client.svelte.js

* Update .changeset/flat-cherries-rule.md

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>

* feat: context

* revert peerDep bump

* revert html variable seperation

* feat: rename

* fix lockfile

---------

Co-authored-by: Florian Lefebvre <contact@florian-lefebvre.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florian-lefebvre florian-lefebvre

Assignees

@florian-lefebvre florian-lefebvre

Labels
pkg: integration Related to any renderer integration (scope) pkg: svelte Related to Svelte (scope)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[@astrojs/svelte] $props.id is identical across islands.
2 participants
@Hugos68 @florian-lefebvre