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

adding svelte-hmr #1223

Merged
merged 11 commits into from
Oct 8, 2020
Merged

adding svelte-hmr #1223

merged 11 commits into from
Oct 8, 2020

Conversation

rixo
Copy link
Contributor

@rixo rixo commented Oct 5, 2020

Changes

This adds svelte-hmr to the Svelte plugin. This allows for component-level, state preserving HMR in Svelte apps.

This is still a little bit WIP, but I could use some help & directions.

Testing

No tests, yet... Any direction about what's needed?

Docs

TODO

Discussion

The main problem I'm facing is that I haven't found how to make svelte-hmr a dependency of the plugin. If I add it as a normal dep of the plugin, I get the following error when launching Snowpack:

[snowpack] Cannot find module 'svelte-hmr/runtime/hot-api-esm.js'

svelte-hmr needs to be installed in the user's project for this to work... so I've just added it as a peerDependency for now.

Any idea how I can fix this? Or do you think it would be best that user keeps the choice to install svelte-hmr or not?

@vercel
Copy link

vercel bot commented Oct 5, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/3r5qld33m
✅ Preview: https://snowpack-git-b8386b248c4a493afcb646ae5d696955bda9aa76.pikapkg.vercel.app

@FredKSchott
Copy link
Owner

Hmm, Cannot find module 'svelte-hmr/runtime/hot-api-esm.js' may be an issue in how you were linking these files during development. When you go to install the package normally, there should be no difference in your node_modules directory between a peerDep and a true dep.

@rixo
Copy link
Contributor Author

rixo commented Oct 5, 2020

Indeed, when installing from file system, it seems to work like a charm :-)

npx create-snowpack-app svelte-app-hot --template @snowpack/app-template-svelte --use-yarn
cd svelte-app-hot
yarn add -D ../snowpack/plugins/plugin-svelte
yarn
yarn start

I'm adding docs about options tomorrow.

@FredKSchott
Copy link
Owner

FredKSchott commented Oct 6, 2020

Yup, ran this myself and it runs well! I see only App.js & App.css being updated in the network panel, the root HMR acceptance at index.js is never triggered. CSS is being replaced correctly as well.

@FredKSchott
Copy link
Owner

mostly just curious: is there any sort of "fast refresh" story for Svelte, where HMR updates preserve component state?

@rixo
Copy link
Contributor Author

rixo commented Oct 6, 2020

Well... I don't have much experience with fast refresh, but I thought svelte-hmr did preserve components state. Doesn't it?

It only preserves state of parent components (easy, they're unaffected) and the component being refreshed itself, but any children of the affected component are recreated with no state transfer whatsoever. That's kind of a technical limitation, it would be prohibitively complicated to try to find & pair child components with their previous self. And very unstable since a component content can very well change entirely upon a HMR update...

Does fast refresh do more than that?

Anyway, you remind me that there is one thing that I have overlooked here. Since the Svelte plugin already had well working HMR for CSS, I left it untouched, but actually svelte-hmr supports "CSS only" updates. I find this very desirable because you can easily end with a lot of CSS only micro edits to a Svelte component, and CSS only HMR is way safer than normal component replacement (good ol' BrowserSync...) -- and they do preserve the state of everything on the page.

Just looked into this, and the only thing I'm missing from Snowpack HMR to enable support for "CSS only" updates is the bubbled info we briefly discussed a while ago. It's still not possible to know that from an accept handler in Snowpack, or did I miss something?

@FredKSchott
Copy link
Owner

Just looked into this, and the only thing I'm missing from Snowpack HMR to enable support for "CSS only" updates is the bubbled info we briefly discussed a while ago. It's still not possible to know that from an accept handler in Snowpack, or did I miss something?

Oh right, can you remind me / link me to this convo again? It's been a while

Does fast refresh do more than that?

You can see this more clearly in the latest Svelte template, if you rebase onto master. When you make a change to src/App.svelte and save, Svelte component state is reset ("This page has been open for 0 seconds"). With Fast Refresh, React/Preact component state is persisted across HMR updates ("This page has been open for XX seconds", unchanged from immediately before HMR update).

That was the behavior I was seeing locally, anyway!

@rixo
Copy link
Contributor Author

rixo commented Oct 6, 2020

I had hidden the link behind a word ;) FredKSchott/esm-hmr#13

And yes, svelte-hmr is supposed to preserve the state of a counter in App.svelte for any change to itself or one of its descendant. State preservation of the modified component can be disabled with an option ('cause it can be somewhat unpredictable sometimes and some people just want stable & fast), but it is opt-in so it should be enabled with the plugin in this PR.

I'm checking with master.

@rixo
Copy link
Contributor Author

rixo commented Oct 6, 2020

The template on master contains a broken reactive statement that resets the counter to 0 on HMR updates... and a memory leak. I've submitted a PR. With that, state preservation should be visible.

@FredKSchott
Copy link
Owner

FredKSchott commented Oct 6, 2020

Thanks for the fix! lol my Svelte ability is clearly lacking, luckily that code has only been public for ~12 hours.

@FredKSchott
Copy link
Owner

Okay, caught up and yea, I'm +1 on the bubbled attribute. Curious: how would you do this otherwise? Is it possible? I don't see an equivalent in webpack so I'm curious.

@rixo
Copy link
Contributor Author

rixo commented Oct 6, 2020

Nice!! bubbled seems to work a treat! Tested with direct changes, or bubbled changes, everything went as expected.

Webpack has no direct equivalent AFAIK. It might be possible to track down the information with global HMR event listeners but I never really tried... CSS only updates is a kind of a luxury feature, so I'm just falling back to full replace without much remorse if the underlying HMR engine doesn't offer it. Actually, thanks to this change, Snowpack is probably the only other system, with rollup-plugin-hot, to support full fledged Svelte HMR.

svelte-hmr has needed a bit of lifting to support the single register accept handlers with this feature. I'm gonna leave it as a prerelease version until I can validate that it doesn't break other plugins, but I think we can go with it for now.

I think I'm good on my side, so removing the draft statut of the PR. Tell me what more do you think we need?

Ah! And be careful, I've merged the bubbled branch in this PR, so I you merge this one you'll get both... That was probably not needed in hindsight 😅

@rixo rixo marked this pull request as ready for review October 6, 2020 23:24
@rixo rixo requested a review from a team as a code owner October 6, 2020 23:24
Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

2 participants