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: html tag hydration: regard empty claimed_nodes array as content mismatch. fixes #6832 #9184

Merged
merged 4 commits into from
Nov 10, 2023

Conversation

kelvinsjk
Copy link
Contributor

@kelvinsjk kelvinsjk commented Sep 6, 2023

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.

Tests and linting

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

Fixes #6832, where nodes were missing on hydration.

Within the claim_html_tag function, the returned claimed_nodes is sometimes an empty array (due to issues with the claim_space function as investigated by rmunn in discussion in #6832). However, hbirler, in the same thread, mentioned that this error in claims should not cause problems in the correctness of the output. Turns out the intended hydration was not working because an empty claimed_nodes array was being regarded as properly claimed nodes rather than a case of mismatch.

I did not add a test as I was unable to recreate the bug outside of SvelteKit (attempts in test/runtime and test/runtime-browser led to passing tests with output different than when I run the Svelte component in SvelteKit). Will be happy to add one if some guidance is provided as to how to recreate this within the test structure

@changeset-bot
Copy link

changeset-bot bot commented Sep 6, 2023

🦋 Changeset detected

Latest commit: 23688da

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

@dummdidumm
Copy link
Member

dummdidumm commented Oct 19, 2023

Thank you! Could you add a changeset and a test? If you add one to runtime it should work, as these are run in hydration mode, too, if not specifically turned off.
One thing I'm not sure about yet is what this means for empty each blocks etc. But maybe a test run reveals that.

@kelvinsjk
Copy link
Contributor Author

Added the test case and changeset.

Thanks for pointing me towards using hydration for the test.

Turns out adding whitespace was what I needed to get a failing test that passes with this PR.

@kelvinsjk kelvinsjk reopened this Oct 19, 2023
@dummdidumm dummdidumm merged commit 0070062 into sveltejs:svelte-4 Nov 10, 2023
6 checks passed
@github-actions github-actions bot mentioned this pull request Nov 10, 2023
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.

Buggy @html tag on page load/refresh
2 participants