-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Conversation
🦋 Changeset detectedLatest commit: 15dc6ec 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 |
39e5fc2
to
94490b6
Compare
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
94490b6
to
15dc6ec
Compare
There was a problem hiding this 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!
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="https://github.com/">Home</a></li><li><a href="https://github.com/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="https://github.com/">Home</a></li><li><a href="https://github.com/away">Away</a></li></ul></nav>`);
export default function App($$anchor) {
$.append($$anchor, root());
} That's a project for future us though. |
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
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 bugsFixes #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
feat:
,fix:
,chore:
, ordocs:
.Tests and linting
pnpm test
and lint the project withpnpm lint