feat: Add uidPrefix option to render
#15403
Conversation
🦋 Changeset detectedLatest commit: e549829 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
|
|
The test brought up a good point - we should have that prefix option for |
Uh you are right...I thought we didn't need that because but it could conflict for that too in astro case |
|
Will attempt, although I only see id_generator being used on the server side? Where are client id's generated? And why isn't there a shared function for this? |
|
We paired on it so I'l let @dummdidumm review it! |
dummdidumm
left a comment
dummdidumm
left a comment
There was a problem hiding this comment.
Looks good to me! Maybe @Rich-Harris can have a final quick look regarding the API
|
Ugh I just realized that we called it |
|
oh yeah that should be aligned |
|
I'll do that real quick, I think |
For me yes |
|
While looking at #15409 it occured to me that we should probably add the option to |
|
@paoloricciuti Could you do that? I have no idea what that means. |
Sure I'll do it later |
|
@Rich-Harris I suggested to put the prefix on the context because this way if you hydrate a component within a component that had a different prefix it will break no? |
|
yeah pretty sure you can't make it global, that won't work with multiple |
|
It's synchronous, so it should be fine, no? |
|
Yeah was just editing my post to say that - it works because it's synchronous. On subsequent updates they won't have the same prefix anymore, but that's also fine because they share one counter. Could use a comment explaining that nuance but otherwise good to go 👍 |
Now that you mention it, what is the purpose of the prefix in the browser, given that the counter is shared globally? |
|
(or are we concerned about multiple copies of Svelte running in the same environment?) |
|
yes, that could happen. It's unlikely but better to be safe than sorry. |
|
Thinking further here - should we just create a random string ourselves, both on the server and the client? Assuming we find something that's sufficiently unlikely to clash, do we even need people to add this option manually when we can deduplicate automatically? @Hugos68 would there anything speaking against that? |
|
I'm not sure about randomising it — that would e.g. prevent SvelteKit from serving 304s. Gah, this is more complicated than I thought. Was really hoping to avoid adding an extra field to component context since those objects are potentially created in their thousands. Hmm |
|
It's dirty but could we use |
Not sure I follow? Why would we need that? Your solution right now works. Between In other words this is good to go IMO |
| // we need to put the `$props.id` after the `$.push` because the `component_context` will be properly initialized | ||
| if (analysis.props_id) { | ||
| // need to be placed on first line of the component for hydration | ||
| component_block.body.unshift(b.const(analysis.props_id, b.call('$.props_id'))); | ||
| } |
There was a problem hiding this comment.
if we don't use context we can revert this change too
paoloricciuti
left a comment
paoloricciuti
left a comment
There was a problem hiding this comment.
Just asking for changes to prevent merging with the fix to move props id after context (and also needs_context = true
| } | ||
|
|
||
| context.state.analysis.props_id = parent.id; | ||
| context.state.analysis.needs_context = true; |
There was a problem hiding this comment.
We can also revert this if we don't use context anymore
|
Btw this was the problem i was talking about with the global value https://svelte.dev/playground/e11bb6e7c46245f3aac2815180d301d1?version=pr-15403 I guess we can fix this if we revert to the old value after mounting the component? |
|
That would not help for subsequent updates of the mounted thing. |
Ohhh gotcha...well it could be a problem if you are filtering IDs based on the prefix. But I suppose is niche enough to not add runtime for this case |
|
I'm all for a better solution, the main use case for me opening this PR and use case is so that individual islands created by Astro can still have unique ID's (so calling |
|
Opened #15428 as an alternative |
|
Closing in favor of #15428, which I'll merge now - thank you everyone! |
4 participants
Before submitting the PR, please make sure you do the following
A better version of #15302
After discovering that
$props.idreturns the same value when used in isolation (for example Astro islands) you will get ID collisions, the users of therenderAPI need to be able to influence the generated ID. This PR adds auidPrefixoption to influence the resulting ID from$props.id(), while still utilizing Svelte's internal UID generator to ensure that$props.idare always unique (in normal situations).Discussed with @dummdidumm here: https://discord.com/channels/457912077277855764/750401468569354431/1344716129268011098
See withastro/astro#13327
feat:,fix:,chore:, ordocs:.packages/svelte/src, add a changeset (npx changeset).Tests and linting
pnpm testand lint the project withpnpm lint