Skip to content

Conversation

@adiguba
Copy link
Contributor

@adiguba adiguba commented Mar 1, 2025

Just a little improvement for the hydration of $props.id(), which I had initially thought of without having the time to do it.

Currently using $props.id() adds a comment like <!--#s1--> at the beginning of the component, for hydration.
So a component like that :

<script>
  let id = $props.id();
</script>
<div id={id}>...</div>

will be hydrated to :

<!--#s1--><div id="s1">...</div>

But in this case, we can retrieve the value of props.id() from the id attribute

With this PR, we check the presence of the $props.id() value on attributes at compile time.
So the hydration comment will be removed :

<div id="s1">...</div>

The hydration comment is still required when the $props.id() value is absent from attributes, or when the component don't starts with a regular element (it's not possible with <svelte:element> because of his own hydration comments).

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.
  • If this PR changes code within packages/svelte/src, add a changeset (npx changeset).

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

@changeset-bot
Copy link

changeset-bot bot commented Mar 1, 2025

🦋 Changeset detected

Latest commit: 611a280

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

@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2025

Playground

pnpm add https://pkg.pr.new/svelte@15409

@paoloricciuti
Copy link
Member

This will likely conflict with #15403 (just putting this out there)...also this strategy feel somewhat risky for some reason 😁 but I'll take a proper look later

@adiguba
Copy link
Contributor Author

adiguba commented Mar 1, 2025

Of course PR 15403 takes priority, but I don't think that this poses a conflict.

@paoloricciuti
Copy link
Member

Of course PR 15403 takes priority, but I don't think that this poses a conflict.

I was talking about literal merge conflicts 😄

I was also just looking at the code...i'm not sure if the removal of a single hydration comment which will likely be there anyway for any component slightly more complex justify the extra complexity of the compilation and the slight runtime overhead of the props_id function. If you look at the SSR's output the diff is minimal
image

I was also wondering if we could take a different approach to this...what if we could include the props.id id in the component hydration marker?

@paoloricciuti
Copy link
Member

I've also tried to create a tinybench with reading text comment+one extra next sibling vs get attribute and the latter is significantly slower so I'm even less convinced is worth it to remove the comment

https://svelte.dev/playground/f96ae09ddfdc45fca8d230d9f48de9af?version=5.20.5

@paoloricciuti
Copy link
Member

However I think we might be able to do something similar to this but kinda opposite which is what I was proposing before: if the first comment is compatible (I think if blocks are not compatible for example) we could stick the id there so that we don't need an extra comment and we need to change the props id function just to know if we need to call hydrate next or not.

I'll try to put up a PR if you don't beat me to it 😁

@adiguba
Copy link
Contributor Author

adiguba commented Mar 2, 2025

I'll try to put up a PR if you don't beat me to it 😁

I tried that without success : too much hydration errors :(

@paoloricciuti
Copy link
Member

I tried that without success : too much hydration errors :(

I was trying right now... didn't got at the client side yet tho

@dummdidumm
Copy link
Member

I think the added complexity and runtime bundle size overhead is not worth this - $props.id() will not be used everywhere all the time, and even if, it's just a lone comment.

@Rich-Harris
Copy link
Member

I agree — it's a creative solution but we already have quite a lot of complexity around hydration, and I worry that the more we add the harder it becomes to make other (potentially more substantial) changes. I'll close this and #15417

@Rich-Harris Rich-Harris closed this Mar 4, 2025
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.

4 participants