Skip to content

feat: POC HMR in Svelte 5 #10100

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

Closed
wants to merge 11 commits into from
Closed

feat: POC HMR in Svelte 5 #10100

wants to merge 11 commits into from

Conversation

rixo
Copy link
Contributor

@rixo rixo commented Jan 7, 2024

I figured a little POC would get the ball rolling...

I have done almost no testing of the HMR behaviour, but I'm still hopeful it should be pretty solid, given this implementation does virtually nothing beside a tiny bit of plumbing and leveraging the existing #key construct.

No more insane hacks.

If we wanted component level state preservation (as opposed to parent level), this would surely not hold true. But previous experience has shown that this feature often proved more confusing than useful, so I'm not sure it would be worth the effort.

Typing is a bit flaky but I didn't find a better way than throwing in a handful of // @ts-ignore. Suggestions for improvements would be welcome.

I'm not sure what we'd want for testing. We probably don't need to test every Svelte feature against HMR, like we did in svelte-hmr, since anything that works with #key should just work the same here. Maybe a test to verify that the HMR option produces the expected code transform, and a single end-to-end test in Playwright would be enough?

A few tests are currently failing on the PR because it changes the default output of the compiler, but I wanted to let the discussion happen before touching everything.

Copy link

changeset-bot bot commented Jan 7, 2024

⚠️ No Changeset found

Latest commit: d410e45

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Jan 7, 2024

@rixo is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

@Rich-Harris
Copy link
Member

Nice! I love the approach. Agree that we can keep the tests fairly basic, and that component-level state preservation probably isn't what we want.

It all looks sensible to me, though I'll confess I haven't fully wrapped my head round our hydration logic so I'll wait for @trueadm to weigh in as to whether anything here is likely to break in future

@trueadm
Copy link
Contributor

trueadm commented Jan 9, 2024

Do we even need to worry about hydration? Can't we just disable SSR when HMR is enabled?

@Rich-Harris
Copy link
Member

Definitely not, that would create a huge breeding ground for bugs

@rixo
Copy link
Contributor Author

rixo commented Jan 9, 2024

Thanks for feedback!

Indeed, for hydration my "logic" is no logic at all. For now, it's really just a hack to pass a hurdle I don't fully understand.

The problem (I have merely observed) is that during hydration the root component receives a null anchor, but our key effect requires an actual anchor to render. If we emulate what a normal component is doing (which works while being root), it kind of consumes the first SSR marker, and then we get hydration mismatch when rendering the actual component.

My hack is a way to get an anchor element without consuming the first marker. Maybe we can find a solution more obvious and robust. Maybe something like a special keÿ function for HMR, or an explicit util to get the root marker and "rewind" the hydration marker. Of, if someone with a better understanding of hydration reckons the current implementation should work for now, even if hackish, we can keep things like this for now, until hydration code evolves and we need to adapt.

Fully disabling hydration would of course be a solution for HMR (actually, even just letting it fail should work currently, since Svelte recovers from scratch in this case). However if someone gets a hydration error without HMR (because the server uses a random source or something), we want them to get the same error with HMR, as much as possible. It's never fun to discover hidden errors when passing from dev to build.

To be clear, my goal is just to replicate hydration behaviour (and errors) immediately after a full reload. People should have a hint that subsequent hot reloads don't behave completely "normally". But a full reload should reset everything to normal for comfortable HMR. We don't want to grow people into the habit of disabling HMR just to clear doubts about errors they're getting.

I'll look into fixing the current tests and what meaningful tests we can add for HMR. We can probably skim over the core rerender logic, since it fully delegates to the existing key construct, but it would be nice to cover accessors, and ideally hydration if it's not too cumbersome to replicate in tests, since we're more deviating from the normal here.

@dominikg
Copy link
Member

Ensuring ssr/hydration warnings on full page load in dev with hmr enabled is important. But like @rixo said, updates sent via hmr won't do the same (it's impossible as they don't have the pristine ssr result available anymore).

If you want to see these for each edit, you have to disable hmr. Might be worth a mention in the docs but even without going into the details it's natural that updating just one components live instances is not the same as doing ssr again.

@dominikg
Copy link
Member

PR to add support to v-p-s sveltejs/vite-plugin-svelte#836

for now this assumes that compilerOptions.hmr is a boolean, the old hot options from svelte-hmr won't be used.

Is the hmr option going to be a boolean flag or are there any "hot" options worth salvaging from svelte-hmr? (eg the annotation comments for keep?)

@dominikg
Copy link
Member

one thing that vite-plugin-svelte currently does to help with "css-only" hmr is that in dev, all elements defined by a component template get the scope hash class applied (by injecting a *{} rule into the style tag), to ensure that when css is hot reloaded, it applies to existing nodes without having to change javascript.

It would be great if the svelte5 compiler did this on its own when for dev+hmr+external css so we can get rid of the workaround in vps.

@Rich-Harris
Copy link
Member

#11106 was merged, so I'll close this — thanks @rixo for getting the ball rolling!

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