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

Hooks for granular HMR support #2377

Closed
ekhaled opened this issue Apr 8, 2019 · 10 comments · Fixed by #3148
Closed

Hooks for granular HMR support #2377

ekhaled opened this issue Apr 8, 2019 · 10 comments · Fixed by #3148

Comments

@ekhaled
Copy link
Contributor

ekhaled commented Apr 8, 2019

Follow up to recent conversation on discord regarding v3 support for HMR.

Let me list out the steps we need for a successful hot reload, and then maybe we can discuss if the core needs to change, or whether we can just monkey-patch stuff.

I'm going to gloss over the component resolution part, as I think we already have that handled in the v2 HMR code.

These steps happen when a hot reload process starts:

Step 1

Get the current state of the component and keep it somewhere.
Looks like component.$$.ctx gives us the whole state.
However, seems like it also returns computed properties, along with handlers and bindings?
Can we work out a way to just get the props (exported or otherwise)?

Step 2

Get the position in the DOM where the component is mounted
We normally add a comment marker just above the component, and store a reference to that comment node.
I think this can be achieved by the compiler using mount_component_dev.
mount_component_dev would first call mount_component, and then store a reference to target and anchor on $$.

Step 3

Destroy the old component
This can be easily achieved using component.$destroy().
We don't need any changes to support this.

Step 4

Render updated component in place of the old component
Here, we can mount the new component with mount_component_dev, using the target and anchor we collected on Step 2.

Step 5

Apply state from old component on to the new component
This I'm not sure at all.
We need to find a way to set the state.
Then we need to ensure that all computeds are up to date.
Will calling $$.update() re-run the computations?


I'll need your help/direction on step 1 and step 5.
I think I can do a PR for Step 2 and Step 4

@Rich-Harris
Copy link
Member

Thanks, this is very useful. The component.$$.ctx stuff is subject to change (see e.g. #2318 and #1922), so I think we should introduce new methods that are explicitly designed for this purpose — e.g. component.$capture_state() and component.$inject_state(captured_state). $inject_state would work basically the same way $set does currently, except that it would inject any local writable vars, not just exported ones.

Yep, $$.update() would recompute stuff, though again, perhaps we should create a dev-mode public method for that.

One thing I haven't wrapped my head round is if there are any subtle timing considerations around bind:this and lifecycle functions...

@ekhaled
Copy link
Contributor Author

ekhaled commented Apr 8, 2019

I think the proposed $capture_state and $inject_state would pretty much cover it.
Of course, along with the dev-mode public version of $$update().

The only thing I forgot mention in the OP is...
Can we bring back _debugName from v2?
It gives us a nice name to put in the comment marker mentioned above, which we can then use in mount_component_dev.

@Rich-Harris
Copy link
Member

Yep, no reason why not

@tomcon
Copy link

tomcon commented Apr 8, 2019

Couple of things here are very useful/desirable in live cases too:
(some of this has cropped up in discord before)

  1. being able to capture complete state
    e.g. for persistence/restoration of components, esp. as apps grow

  2. dynamically changing the mount point of a component at runtime
    e.g. embedded edit form that can be reused in a list view, shown in situ under item being edited

If for some reason one or both of these are not surfaced as part of the public api, then to have them unofficially available would be very much appreciated, pretty please

@ignatevdev
Copy link

Any news on this issue?

@hitsthings
Copy link

hitsthings commented Jun 11, 2019

Using webpack I get sapper-dev-client.js errors on if (module.hot.status() === 'idle') { in Firefox because module.hot is undefined. Can be annoying when you break on exceptions.

Edit: this is when using the Sapper template which sets hotReload: false and links to this issue.

@TrungRueta
Copy link

if clone sapper template webpack then run npm run dev or sapper dev, project still failed when try update view after code change. I saw in webpack.config.json we have 2 lines commented about hot-replace issue, but it not enough to avoid error when develop with webpack...

from command to start dev mod: sapper dev we also need disable sapper hot reload feature to let sapper fallback live reload feature. Command should be : sapper dev --no-hot.

Maybe update template a hotfix, but write note somehow so everyone can temporary avoid error until hotreload work in v3 ?

@mdempsky
Copy link
Contributor

mdempsky commented Dec 18, 2019

Why was this issue closed? Is there a better issue for tracking HMR support? (Edit: I misread the closed message; I see now it was closed because #3148 was merged.)

I ask because this issue is referenced in sapper-template: https://github.com/sveltejs/sapper-template/blob/23b93f2c12dd387de62385bfafdaa89cd5f90430/webpack.config.js#L27

It seems like either: (1) the issue should be re-opened; (2) sapper-template comments should be updated to refer to a different blocking issue or explanation; or (3) hot-reload should be re-enabled.

@rixo
Copy link
Contributor

rixo commented Dec 20, 2019

@mdempsky There is #3632 that is currently opened and tracks HMR support generally.

The Sapper template should probably be updated. You can make a PR if you feel like it.

There's no official HMR support for Svelte 3 yet, but I've got a prototype that's been doing pretty well. You can find links to all my relevant plugins & templates in this readme. This implementation is experimental because it relies on Svelte's internal APIs, and so it breaks sometimes with new versions of Svelte (currently the case with 3.16 unfortunately... fix is coming!). Some of the hooks needed to fix this were addressed by this issue, and work on the rest in ongoing.

@benmccann
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants