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

HMR hooks #3148

Merged
merged 6 commits into from
Sep 8, 2019
Merged

HMR hooks #3148

merged 6 commits into from
Sep 8, 2019

Conversation

Axelen123
Copy link
Contributor

This PR adds hooks needed for HMR support and should fix #2377. Here is a Webpack Demo.

If in the demo you get an error like:

ERROR in ./src/App.svelte
Module build failed (from ./node_modules/svelte-loader/index.js):
Error: Cannot find module 'css-tree/lib/parser/index.js'

Then you should run:

git clone "https://github.com/Axelen123/svelte.git#hmr"
cd svelte
npm install
npm link
cd ..
npm link svelte

@Rich-Harris
Copy link
Member

This is extremely cool! Apologies for the long wait before getting to this PR. This might actually get me using webpack (or it might get me to try and get HMR in Rollup...)

@Rich-Harris
Copy link
Member

I'll confess I haven't been following the related work on svelte-loader and svelte-dev-helper — what needs to happen before people can start using this?

cc @ekhaled 😀

@Rich-Harris Rich-Harris mentioned this pull request Sep 9, 2019
@rixo
Copy link
Contributor

rixo commented Sep 10, 2019

Sorry for long post. Please don't get discouraged by length, it's a light read!

The demo in this PR is based on my fork of svelte-loader.

The principle of my fork is to liberally abuse Svelte's private API in order to emulate the hooks that are the object of this PR. My immediate goal was to unlock as much HMR features as possible with this approach, and the ultimate goal was to inform the implementation that would eventually make its way into Svelte.

Sorry for not posting that earlier -- I didn't thought it would get merged before the discussion resumes... But I'm affraid these hooks won't get us all the way for HMR support.

Let's first consider the goals for HMR. What we want is to preserve (transfer?) the following pieces of state from one component to another:

First tier

  1. location in DOM (aka insertion point, aka target / anchor)

  2. public props

  3. bindings

  4. slots

  5. event & lifecycle callbacks

  6. context

  7. actions

Second tier

  1. local state (local variables in the component's instance function)

That's what I have identified so far, maybe I've missed something important.

The first tier is what I consider to be the essential features of HMR. As a HMR user, anything missing from this list would lead to a sentiment of yet another broken HMR.

The second tier is just a nice to have. It might be very useful for some people, in some situations (e.g. to keep modals / dropdown / etc. open across HMR updates). But my intuition is that it might also get complicated fast.

Anyway, regarding this PR, I failed to run the code but if I read it correctly (please, correct me if I'm wrong), the PR only covers props, and possibly actions and slots (1 / 3 / 6). If that's the case that means we're still missing, bindings, callbacks & context (2 / 4 / 5) for HMR. And local state (7), if that's worth pursuing right now.

My understanding was that, if we want to isolate Svelte's private concerns, these hooks will have to handle the whole state of the component, not just props.

Furthermore, the HMR Proxy implementation that is used in the demo has some special needs that are also covered by abusing the private API currently, so Svelte should probably expose something to handle these too:

  • The proxy needs access to lifecycle callbacks on mount & destroy. We can't use regular { onMount, onDestroy } from 'svelte' because HMR code can't run from inside the init function. We could monkey wrap the $destroy method, but it is not called for nested components.

  • The hacked mount hook is also used to know the anchor & target used to render fresh component versions. But despite abusing the private API, this still breaks when elements are moved in the DOM (keyed lists for now). I don't think insertion point should be a responsibility on HMR side.

Then, there's the question of context, that the capture / inject strategy can't handle at all.

HMR's heart boils done to something like this:

function updateComponent(MyComponentFresh) {
  // cmp is the proxy's private instance of MyComponent
  const state = cmp.$capture_state()
  cmp.$destroy()
  cmp = new MyComponentFresh({ target, anchor })
  cmp.$inject_state(state)
}

Here, context will be broken for new MyComponentFresh because {current_component} from 'svelte/internals' is not currently set.

At this point, there's nothing $inject_state can do to save the day because the user's instance function has already been called in this wrong context. Maybe it has crashed on an undefined reference, bringing down the whole HMR session, or maybe there was a console.log in there that will drive the unsuspecting user crazy.

console.log(getContext('user')) // undefined => WTF? Svelte's broken?

const username = getContext('user').name // CRASH!

And god knows what might have happened when I called $destroy -- or what will happen when all the outros and async cleanup have completed and some destroy callbacks will be triggered at my unsuspecting proxy.

There's too much happening, too far appart, in these two lines:

cmp.$destroy()
cmp = new MyComponentFresh({ target, anchor })

HMR code can't even try to mitigate anything because everything happens synchronously before it regains control, and it all happens behind the closed doors of the init function. Svelte can't help because it doesn't know what we are trying to do, and it doesn't have access to the previous component for plucking the state to transfer.

In the end, I don't think the $capture_state strategy draws the right line for our use case. By requiring HMR code to destroy and instantiate new components itself, it gives it too much responsibility that it is not in a position to assume without forbidden knowledge of Svelte implementation details (insertion point, context...). On the other hand, Svelte could handle it like a champ but is not given the opportunity. It feels more like cutting a concern in half than separating distinct concerns to me.

That's why I suggest it might be saner to settle for a single do-it-all method like this:

cmp = cmp.$replace(MyComponentFresh, probablySomeOptions)

// or maybe async, but best avoided if possible (in regard to HMR implem)
cmp = await cmp.$replace(MyComponentFresh, options)

It is a little more upfront complexity for Svelte, but at least this complexity is located where it can be best handled: by Svelte devs that are intimate with the intricate details of component initialization, and who directly have the hand for propagating evolution.

And who could unilaterally decide to put HMR support on hold for some hot cooking versions simply by throwing an Error that says: "Sorry, HMR not supported in this version of Svelte. Please disable it or downgrade to last major for now." (Instead of confusing HMR mess, that's a big win!)

On the plus side: instead of subtly differently brittle HMR gluing code in each bundler, we get easy to implement, robust & uniform HMR for everyone (that can ;p).

@Rich-Harris What do you think?


Links

My fork of svelte-loader

The most relevant file to this discussion is lib/svelte3/hot/svelte-hooks.js (linked above). It encapsulates all the private touching and public emulation.

There are two interesting branches:

  • #hmr sticks to the $inject_state / $capture_state approach

  • #hmr-next experiments with the $create approach (and has less bugs)

My PR on svelte-loader (on #hmr branch)

Original demo (without public hooks)

Svelte Native demo

@rixo
Copy link
Contributor

rixo commented Sep 13, 2019

what needs to happen before people can start using this?

In a more "moving forward" spirit... IMHO the right move right now would be to work & merge the monkey patched code into svelte-loader and release that with a bêta tag or something.

This will leave some time to calmly assess the situation here, and move deliberately regarding the evolution of Svelte dev API.

Meanwhile, people impatient with HMR will get to see something, and to help us to start collecting precious feedback.

Not sure we can achieve zero-bug without the hooks but still, it's not the worse HMR experience I've had. It's already been working well enough for me in my Sapper app, for a few months. I've heard @halfnelson had some success with Native too.

hmr-spec

(To be clear, "pending" means "failing -- and I have no idea how to fix it")

Then, in a collective & cheerful effort, we can identify the tricky bits of the dev hooks implementation with regard to HMR, and consolidate this knowledge into a test suite.

And so, when times come, we will have all we need to serenely move the required parts into Svelte. ☮️

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.

Hooks for granular HMR support
3 participants