Skip to content

fix: ensure hydration walks all nodes #12448

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

Merged
merged 1 commit into from
Jul 16, 2024
Merged

fix: ensure hydration walks all nodes #12448

merged 1 commit into from
Jul 16, 2024

Conversation

dummdidumm
Copy link
Member

We've marked several methods used for walking the DOM with a __NO_SIDE_EFFECTS__ comment. That was good historically, because we didn't need those kept around if its results were unused, but since the hydration changes in #12335 this actually introduces a bug: Because that PR now relies on the hydration nodes being correct due to walking the DOM, tree-shaking unused variables/calls results in the walk being incorrect, leading to bugs

Fixes #12422

No test because we would need to treeshake stuff for it to be testable

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

Copy link

changeset-bot bot commented Jul 15, 2024

🦋 Changeset detected

Latest commit: 15dc6ec

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

We've marked several methods used for walking the DOM with a `__NO_SIDE_EFFECTS__` comment. That was good historically, because we didn't need those kept around if its results were unused, but since the hydration changes in #12335 this actually introduces a bug: Because that PR now relies on the hydration nodes being correct due to walking the DOM, tree-shaking unused variables/calls results in the walk being incorrect, leading to bugs

Fixes #12422
Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

Makes complete sense. I was also looking into a similar bug and this fixes that too!

@Rich-Harris
Copy link
Member

Ah yep, this occurred to me the other day but I was far from a laptop and so completely forgot about it. Sorry!

I do think we can probably be more conservative about which nodes get walked. A component like this...

<nav>
  <ul>
    <li><a href="/">Home</a>
    <li><a href="/away">Away</a>
  </ul>
</nav>

...becomes this...

import * as $ from "svelte/internal/client";

var root = $.template(`<nav><ul><li><a href="/">Home</a></li><li><a href="/away">Away</a></li></ul></nav>`);

export default function App($$anchor) {
	var nav = root();
	var ul = $.child(nav);
	var li = $.child(ul);
	var a = $.child(li);

	$.reset(li);

	var li_1 = $.sibling(li);
	var a_1 = $.child(li_1);

	$.reset(li_1);
	$.reset(ul);
	$.reset(nav);
	$.append($$anchor, nav);
}

...when an ideal world we'd figure out that there's nothing to do with the child nodes, and generate something more like this:

import * as $ from "svelte/internal/client";

var root = $.template(`<nav><ul><li><a href="/">Home</a></li><li><a href="/away">Away</a></li></ul></nav>`);

export default function App($$anchor) {
	$.append($$anchor, root());
}

That's a project for future us though.

@Rich-Harris Rich-Harris merged commit da6e192 into main Jul 16, 2024
9 checks passed
@Rich-Harris Rich-Harris deleted the hydration-fix branch July 16, 2024 01:38
trueadm pushed a commit that referenced this pull request Jul 16, 2024
We've marked several methods used for walking the DOM with a `__NO_SIDE_EFFECTS__` comment. That was good historically, because we didn't need those kept around if its results were unused, but since the hydration changes in #12335 this actually introduces a bug: Because that PR now relies on the hydration nodes being correct due to walking the DOM, tree-shaking unused variables/calls results in the walk being incorrect, leading to bugs

Fixes #12422
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.

Production build failure of svelte@5.0.0-next.179 -> svelte@5.0.0-next.182
3 participants