-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add basic HMR support for Svelte 3 (and svelte-native) #107
Conversation
update native support
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.
Adding for #124
|
||
const { version } = req(`${base}/package.json`); | ||
|
||
const major_version = +version[0]; |
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.
const major_version = +version[0]; | |
const [major_version, minor_version ]= version.split('.'); |
major_version >= 3 ? require('./svelte3/make-hot') : require('./make-hot'); | ||
|
||
module.exports = { | ||
major_version, |
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.
major_version, | |
major_version, | |
minor_version, |
? require('svelte/compiler') | ||
: require('svelte'); | ||
const { | ||
major_version, |
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.
major_version, | |
major_version, | |
minor_version, |
What's causing the tests to fail? |
@DominusVilicus I think I incorrectly removed svelte from package.json... Anyway, I don't think this PR is current anymore. HMR implementation has changed a lot and integrating will need a fresh look. Closing this one to reduce confusion. My goal is to keep |
No probs. Mention me when you add the new HMR implementation in a PR, would be happy to review. |
It is not really planned to integrate HMR into the official plugins short term. For now the plan with HMR is increase adoption, get confidence, stabilize and then make it official. That being said, the plan is also to keep the hot plugins in sync with official ones. So like I said, anything that makes it here will be merged in svelte-loader-hot too (feel free to PR when you feel this process is lagging, I'm a bit behind tracking all this stuff ^^). |
This is a rework of the experimental code for Svelte 3 HMR I've presented in #92.
The loader can be tried with this demo app.
The rework builds around existing code instead of replacing it, in order to preserve existing support for svelte 2 + HMR. Also, this offers a more readable diff, as suggested by @ekhaled in sveltejs/svelte#3126 (comment).
Actual HMR support works for basic cases. Not all cases unfortunately... Outros, for example, are known to break HMR currently. I am working in parallel on a test suite for HMR, to identify, fix & consolidate such cases.
But my goal here is more to get the ball rolling, foster some discussion and, hopefully, some decisions...
What would be great would be to be able to put version numbers on this ongoing work soon, so that people can pin versions of their stack when it works. This is not possible when installing the package from a git branch, and it is a pity because Svelte is evolving fast, under-development HMR will break often... And sometimes people just want to work on their actual project, not on fixing their HMR (or worse: loader) that was working before.
This implementation emulates svelte's
capture_state
andinject_state
hooks as proposed in sveltejs/svelte#2377. My hope is that this work will usefully inform the actual implementation of HMR hooks in Svelte, when time comes. So I've moved all the emulated code / code that touches svelte private API into their own file to help with that.