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

chore: remove redundant hydration code #9698

Merged
merged 6 commits into from
Nov 30, 2023
Merged

chore: remove redundant hydration code #9698

merged 6 commits into from
Nov 30, 2023

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Nov 29, 2023

Turns out we don't need to set $$fragment to undefined anymore. Also noticed some suspect code just below it that should probably be fixed too.

Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: 317de4a

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

chore: remove redundant hydration code
Copy link

vercel bot commented Nov 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
svelte-5-preview ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 30, 2023 10:11am

@dummdidumm
Copy link
Member

Where do we set it do undefined then? Because we do set it, and to avoid memory leaks, we should unset it somewhere, right?

@dummdidumm
Copy link
Member

Somewhere in line 1400 at render.js I vaguely remember having run into this, and checking explicitly for null there. That may be needed to adjusted, too.

@trueadm
Copy link
Contributor Author

trueadm commented Nov 29, 2023

Somewhere in line 1400 at render.js I vaguely remember having run into this, and checking explicitly for null there. That may be needed to adjusted, too.

Nice. Removed that now as it shouldn't be needed.

Where do we set it do undefined then? Because we do set it, and to avoid memory leaks, we should unset it somewhere, right?

get_hydration_fragment used to assign the fragment to the node at one point I guess? We can re-explore memory leaks but this doesn't ever get set anymore.

@dummdidumm
Copy link
Member

dummdidumm commented Nov 29, 2023

get_hydration_fragment used to assign the fragment to the node at one point I guess? We can re-explore memory leaks but this doesn't ever get set anymore.

That's not true, $$fragment is still set within capture_fragment_from_node in operations.js. I therefore think it should be set back to undefined somewhere once it's no longer needed.

@trueadm
Copy link
Contributor Author

trueadm commented Nov 29, 2023

@dummdidumm Yeah, so get_hydration_fragment used to use next_sibling, but at some point it was changed to .nextSibling. The next_sibling logic triggered capture_fragment_from_node.

@dummdidumm
Copy link
Member

I don't fully understand what you mean by that sentence. What I mean is, that there are still usages of $$fragment, and now it's not set to undefined anymore anywhere, and that seems like a potential memory leak.

@trueadm
Copy link
Contributor Author

trueadm commented Nov 29, 2023

@dummdidumm Pushed some cleanup logic.

'svelte': patch
---

chore: remove redundant hydration code
Copy link
Member

Choose a reason for hiding this comment

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

This might not be an accurate description anymore now that it's adding rather than removing code. I don't care so much about the changeset, but just thought it was funny 😆

Screenshot from 2023-11-29 20-19-53

@dummdidumm dummdidumm merged commit 2e461eb into main Nov 30, 2023
9 checks passed
@dummdidumm dummdidumm deleted the cleanup-hydration branch November 30, 2023 10:14
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.

3 participants