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

Remove Svelte 2 HMR #153

Closed
wants to merge 1 commit into from
Closed

Remove Svelte 2 HMR #153

wants to merge 1 commit into from

Conversation

benmccann
Copy link
Member

@rixo this was suggested to me in #150 (comment). Do you think this PR to update the README to point to your repo is a good idea?

I'm afraid I always forget what the latest state of HMR is, what support we would need to build an official interface in core, etc. I wanted to know your thoughts on this PR and whether it's a helpful step to take. I figure it doesn't do anyone any good to have a broken version in and then we'd have a clean slate where if we add your stuff in hopefully it will be easier for you since you wouldn't have to worry about making breaking changes. Or should we just try to merge your stuff in here now instead? I haven't talked to any of the other maintainers about what the best path forward is yet, but thought I'd get a discussion going on the topic

@benmccann benmccann force-pushed the rm-hmr branch 2 times, most recently from c0cea2a to 5113ac5 Compare January 13, 2021 21:24
@non25
Copy link
Contributor

non25 commented Jan 14, 2021

@benmccann Personally I would merge svelte-loader-hot changes here and tell people in the readme that you can use it, but it is experimental.
The only thing you can do right now on svelte-loader with hmr is to disable hotReload right away, so it doesn't break runtime with cryptic error message.
Wouldn't hurt to merge working hmr here, albeit experimental.
That way we could incrementally improve it or at least start to pinpoint what is needed to make it production-ready with help of our users.
npm stats difference is huge: 14,034 vs 845 weekly downloads.
If you are interested I could make a branch that merges svelte-loader-hot here.

I think webpack is still useful, in certain scenarios it is very handy to have its established ecosystem and configurability.
Also now it is possible to use cache-loader in webpack 4 and cache subsystem in webpack 5 after that PR you've just merged.

I would also update template-webpack to webpack 5, especially when we got compatibility problems out of the way.
Would be also cool to make template-webpack with branches, I have cool ideas about tailwindcss that could use @apply in sfc's and have bearable dev reload time: sveltejs/svelte-preprocess#275 (comment).

What do you think ?

@benmccann
Copy link
Member Author

I would be interested to see what changes would be necessary here to support working HMR and open to that idea

@non25
Copy link
Contributor

non25 commented Jan 14, 2021

Should we use svelte-hmr commons as a dep, or should we extract needed code from it to make svelte-loader more self-contained ?

@benmccann
Copy link
Member Author

It seems like a good idea to leave it as a common library assuming it would be shared by the Rollup plugin, etc. eventually

@rixo
Copy link

rixo commented Jan 14, 2021

I'm also of the opinion that it's time to kill the hot forks and integrate svelte-hmr into official plugins, to focus effort, and increase adoption. It makes even more sense here than in the Rollup plugin, since Webpack does officially support HMR.

I would personally drop the "experimental" language, in favor of something like "community supported". svelte-hmr has been around for quite some time now, and I believe it's close to be as stable as HMR can be. Also, maintenance is essentially guaranteed, since I'm committed to fix any breakage that may happen following future evolution of the compiler, and I'm similarly extremely confident that changes to the compiler would be accepted if need be. (I wouldn't take offense if we keep calling it experimental though.)

The hot fork has not diverged much. I've merged it into the branch of this PR for illustration: https://github.com/benmccann/svelte-loader/compare/rm-hmr...rixo:svelte-hmr?expand=1

I've not tested it, and it would probably need to be updated to last version of svelte-hmr, but it gives an idea.

There's more changes involved than with the Rollup plugin because Webpack needs an adapter for module.hot to import.meta.hot, that svelte-hmr natively supports. Here again, I believe I saw some news that Webpack HMR was evolving, like moving to import.meta.hot too, so maybe it should be updated too. @non25 As a current Webpack user, any idea about that?

And indeed, we need to keep the dependence on svelte-hmr. Fixes still happen there and, as mentioned, we want an even HMR experience across Rollup/Nollup, Webpack, Snowpack, Vite, and anyone who might want to join the dance. The HMR transform should be provided by the compiler to similar effect, eventually.

@benmccann
Copy link
Member Author

I'm happy to close this PR and review yours if anyone wants to send one against master to add HMR

@non25
Copy link
Contributor

non25 commented Jan 15, 2021

Superseded by #156.

@benmccann benmccann closed this Jan 16, 2021
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