Reference: Vite #7
Comments
|
@yyx990803 oh nice! For some reason I was under the impression that your HMR implementation leveraged Rollup internally, not sure why I thought that. Any interest on collaborating on this then? We're kind of working backwards from "working implementation" towards "well-documented spec", so it's still very early. from a quick look at your implementation, it looks like there's a few vue-specific things that make sense for Vite but not for a generalized implementation, which could be a blocker unless it could sit nicely on top of a generalize spec. I'd also love to understand the limitations/tradeoffs of your event bubbling strategy. I think we just got something working in FredKSchott/snowpack#335 (review) that could be interesting for Vite, would love to get your thoughts on that if you have any. |
|
also lol where are all these 👍 reaction emojis coming from? 👋👋👋 |
Disclosure: I shared this repo with the Vue team since it's relevant to what we are working on. |
|
Gotcha, yea I was aware of how you did event bubbling but curious how it compared to what we've implemented. But, I realize I also didn't provide much details about what we've implemented. Let me write up how our event bubbling works and the can ping you for feedback. If you're interested in collaborating on this together, let me know! I'd genuinely love to if we can align on a common design and save us both some duplicate implementation time. From a quick look at vite, the differences we'd want to iron out are:
|
👍 Getting rid of tool-specific imports is a good idea. I see it's now been updated to get rid of the import + manual context creation and directly use
If we move to
This is supported in webpack's HMR, and in the past it's been useful in the cases of e.g. a global state store that can hot swap its sub modules.
I've never found a use case for these two APIs to be honest (after supporting HMR for Vue, React and Preact), but they should be fairly easy to support. It seems |
👍 Makes sense to add for Snowpack & ESM-HMR.
In Webpack's implementation both events are supposed to bubble up to the parent, so they act more like forcing a "file changed' from the client instead of from the server file watcher. But at the moment we go for the naive solution and just force a refresh.
This use-case makes sense in theory, although I agree I haven't seen it in practice either and would be fine to leave this behavior undefined for now (ex: "the ESM-HMR client MAY bubble this event up to be handled by a parent, or refresh the page) |
|
As far as I can tell those are the only differences, so it feels like we're actually not far off here! I added some more details to the README, so it might make sense for you to look over those and make sure I didn't miss anything about Vite. Would a shared TypeScript type declaration be a good artifact to come out of this, that we could both pin to? |
Would it then make sense to maybe leave them out of the spec for now to reduce the maintenance surface (since users can simply call
Sounds like a good idea. There is currently one in Vite: https://github.com/vuejs/vite/blob/master/hmr.d.ts - we just need to make a version that augments the Note Vite currently has a custom |
Of course right after I write this, I find a use case in react-refresh that I believe both of us want to handle:
I like the idea of a server being allowed to implement this more advanced handling, but not being required to. I think that's a good middle ground for HMR implementers that also protects the HMR consumer from having to leave the sandboxed HMR API to resolve a bad update themselves with a more bare-metal If you feel strongly the other way, I'm okay with removing for now and then revisiting in a couple of weeks when we tackle that React Fast Refresh use case mentioned above.
👍 |
|
I'm actually ok with including them if there are valid use cases, although in this case it seems we can simply skip the |
|
The scenario would be an exported non-component function added to a module after-the-fact (file goes from all React components to 1+ non-React component exports). Does Vite's implementation handle that? Snowpack's current implementation couldn't without a way to conditionally accept the update. |
|
Oh I see, yeah it would require updating the accept data on the server. I think it makes sense to include them then. |
|
If the interface makes sense so far, then I'm curious to talk about implementation details. I think I found an difference that affects how we would implement dependency whitelisting for the accept call, based on https://github.com/vuejs/vite/blob/59da38c185b428a178d320b8bd5187b34bd942aa/playground/testHmrManual.js. No Dependencies Whitelist
Not a huge difference, assuming you were okay with scoping the module property and adding support for data passing. Dependencies Whitelist
So one thing that appears special in Vite is passing the dependencies of a module to the accept call of the parent. How important is that use-case for you? This breaks a few key assumptions that we've built on top of (mainly, that an accept call always accepts an update to the current module) that would be very difficult to unwind to implement on our side. I don't see this implemented in other HMR specs, so curious to hear more of the reasoning for this if it's something that you need. |
|
Re dependencies whitelist usage: I'm not sure how your implementation goes, but if we are not replacing the accepting module, and not passing the new accepted modules via the callback, then you need to be updating the actual import bindings - which requires rewriting the imports of accepted deps to e.g. // main.js
import foo from './foo.js'
import.meta.hot.accept('./foo.js', () => {
// if main.js is not replaced, then the `foo` binding would be stale
// unless it's somehow rewritten into a let binding and updated behind the scene
})I actually think accepting the dep whitelist via the callback is more explicit and more consistent with the self accepting usage. |
|
Totally, this may be a difference between our implementations. Right now, in your example: // main.js
import foo from './foo.js'
import.meta.hot.accept(['./foo.js'], ({module}) => {
// Not shown: updating main.js to accept this update
})
|
|
Yeah, so here's a use case we've had quite often for the whitelist usage (pseudo code based on Vuex usage, but applies to Redux as well for // store.js
import moduleA from './modules/a'
import moduleB from './modules/b'
export const store = createStore({
modules: {
a: moduleA,
b: moduleB
}
})
import.meta.hot.accept(['./modules/a', './modules/b'], ([newA, newB]) => {
store.replaceModules({
a: newA,
b: newB
})
})The key here is the store is exported and used by the application. Preserving the current |
|
That makes a lot of sense, and I can't think of any other good way to handle that otherwise without re-exporting your imports to grab them in the callback, which feels gross. How does this work with event bubbling? If |
In Vite's current implementation, |
|
Awesome, I'm +1 to add to the spec. |
|
@yyx990803 support for what you've described has been added. Based on what you've said, I think that's the last bit of Vite HMR that wasn't covered here, other than any Vue specific stuff which would be out of scope for this generalized proposal. The last step would be on your end: to move to |
|
So I noticed a few differences while looking at the latest spec, and I propose some slight syntax changes regarding the callback signature. Currently the spec requires accessing the reloaded module as a nested property on the object passed to the callback. This might make sense if the callback is accepting many different things, but in the context here there are really only two categories: the updated module(s), and the persisted data. Considering usage of Proposed Changes (updated based the following comment): Self Accepting// current spec
import.meta.hot.accept(({ module: { foo }, data }) => {
// ...
})
// proposed change
const data = import.meta.hot.data
import.meta.hot.accept(({ foo }) => {
// ...
})Deps// current spec
import.meta.hot.accept(['./depA', './depB'], ({ deps: [depA, depB], data }) => {
// ...
})
// proposed change
const data = import.meta.hot.data
import.meta.hot.accept(['./depA', './depB'], ([depA, depB]) => {
// ...
})In addition, the spec doesn't seem to support single dep via string, which is supported in webpack: // proposed addition, not currently covered in spec
import.meta.hot.accept('./depA', (newDepA) => {
// ...
}) |
|
Follow up: based on webpack's implementation and https://github.com/rixo/rollup-plugin-hot - both attaches persisted data on the If we simply access the persisted data via |
|
Okay, I'm on board with moving data out of the callback. Since the On the flip side, the more verbose API is intentional:
Supporting 4 different interfaces here is a flaw in Webpack's original design worth tackling. Going back through some Webpack history, this wasn't an intentional design, it's just how the API had to evolve to avoid breaking changes in the 7 years since it was originally implemented. With 7 years of HMR hindsight and no legacy users to worry about, this is the right time to tackle this clean up. Given that this is an advanced API, explicit & concise are more valuable (and easier to document) than implicit & having-many-permutations. |
|
This may come down to personal preference but I don't really see a problem here - |
I think the implementation still needs to ensure the same |
Good point, this is given for free by our implementation but should still be called out as an explicit requirement for a valid ESM-HMR implementation. |
|
What do you think about import.meta.hot.data
import.meta.hot.accept()
import.meta.hot.accept((selfModule) => {})
import.meta.hot.acceptDeps(['./depA', './depB'], ([depA, depB]) => {}) |
|
I feel like the current design hits the right balance between the max-verbose and the max-dynamic designs you've outlined:
|
|
Even though the callback signature is the same, the values in the object being passed in changes based on the 1st argument. The Having two separate methods makes the intention more explicit (it's easier to look at what method is being called than looking at the arguments), and also makes the usage less verbose. |
|
In the documented API, |
accept(({ module, deps }) => {
// deps is empty
// module is the relevant value
})
accept(['./depA'], ({ module, deps }) => {
// deps now non-empty and is the relevant value
// and do we really need module here if self module is not replaced in this case?
})Compare to: accept((selfModule) => {
// selfModule is the relevant value, because we are self-accepting
// self module is re-instantiated
})
acceptDeps(['./depA'], (deps) => {
// deps is the relevant value because we are accepting deps
// self module is NOT re-instantiated
})I guess personally for me it feels weird for an API method to be overloaded with different signatures and then you are expected to retrieve the relevant value from different properties under the object passed to a callback. I don't think I've come across many APIs designed like that before. If we want to be explicit, I would much prefer two API methods for self-accepting vs. accepting deps, since they are fundamentally doing different things (especially when there are important behavior differences in terms of self module re-instantiation). |
|
Does this help? accept(['./depA'], ({ module, deps }) => {
// deps & module could both be relevant to your use-case.
// self module is always updated.
})
On the flip side we simplify documentation, interface, and implementation with this less complex implementation. |
|
Based on what we've discussed here and the example here, when you are accepting deps, the self module should not be re-instantiated. This is necessary to preserve the identity of the exported store instance. Otherwise the store replacement example will not work, because you are calling |
|
Whoops, I read too quickly. That example is actually fine as is: you would only want to update the current module's store and could ignore the newly created module instance's store. Or, you could also: import moduleA from "./modules/a.js";
import moduleB from "./modules/b.js";
export let store = import.meta.hot.data.store || createStore({
modules: { a: moduleA, b: moduleB },
});
import.meta.hot.accept(
["./modules/a.js", "./modules/b.js"],
({ module, deps }) => {
store.replaceModules({
a: deps[0].default,
b: deps[1].default,
});
}
);
import.meta.hot.dispose(() => {
import.meta.hot.data.store = store;
});If what you're looking for is something that listens to update events in your dependencies, I think there could be a place for that. Something like
To be clear, these are all problems that Webpack/Rollup/System.js don't have since they can hot-swap a module out with an update. But with ESM we need to self-accept updates to make sure that we never have more than one copy of the module running in the application, since a dependent can't update dependency on its behalf. |
On your next edit, the The example using
You don't - data passing is only relevant for self accepting modules that perform side effects on update. In this case, the deps modules (or reducers) should be side-effect free (and therefore is safe to be hot replaced)
In Vite's implementation, we walk the import chains of any changed module - and only perform hot updates if all import chains reach a hot boundary. If there are any importer of the changed module (even up the chain) that is not a boundary (i.e. has no explicit logic to handle the update), the app will be reloaded in full. This guarantees there will never be stale references. If you do not reload the page in full, then re-instantiating the self module with its deps doesn't really get rid of the stale reference problem. For example, if there is another file in your app that directly imports |
Maybe this is a misunderstanding of the spec: the accept handler is the only way to apply updates from the new instance to the existing instance that's connected into your application: // store.js
import moduleA from "./modules/a.js";
import moduleB from "./modules/b.js";
export const store = createStore({
modules: { a: moduleA, b: moduleB },
});
import.meta.hot.accept(
["./modules/a.js", "./modules/b.js"],
({ module, deps }) => {
store.replaceModules({
a: deps[0].default,
b: deps[1].default,
});
}
);
Just to make sure I understand you 100%, in Vite does a change inside of /* store.js */
import moduleA from "./modules/a.js";
import moduleB from "./modules/b.js";
acceptDeps(["./modules/a.js", "./modules/b.js"], () => { ... });
/* foo.js */
import moduleA from "./modules/a.js";
import moduleB from "./modules/b.js";
// No accept handler |
|
I guess the misunderstanding probably roots from the difference in our implementations... wouldn't this new instance of
Yes, it would be a full reload because |
|
Given there seem to be many undefined behavior differences between our implementations, I'm not sure if it's worth it to bend them to conform to the exact same spec. Even if we achieve syntactical consistency, there are probably more fundamental differences in terms of actual behavior. |
|
Sorry, got bogged down with 2.0 release work. Yea, I guess that's fair, maybe 1:1 compatibility shouldn't be the goal here if you already have an implementation that you're happy with. I definitely appreciate where we've come together on this already though. maybe stepping back from the interface/implementation itself: I'm genuinely curious how you got self-accepting modules to work in a way that also replaces the current instance of the module in the application. The "every module must be self-accepting" design of Snowpack's HMR is based off of this requirement: since we don't own the module system, we need to always be updating the first instance of the module itself since that's the one that will always be referenced inside the rest of the application. If you have the time, I'd love to hear how that works in Vite (or, if that's documented somewhere, a reference to the docs would be great too). |
|
Heyho, I see the need for a different HMR API for non-bundlers, as the webpack HMR API won't work for native ESM. I totally understand why you are creating a new spec for that. But could you do one thing: bind to a different name for it: e. g. If you want to know the differences:
|
|
@sokra is webpack using FWIW Vite is not 100% conforming to this spec (AFAIK snowpack is the only implementation of this), but it seems more reasonable for |
Is this a useful goal? Parcel, Webpack, and Rollup (plugins) all include different Thanks for taking a look btw! |
|
Looks like webpack landed its API as |
|
we tried to follow the client interface of this repo in my implementation of hmr over at the modernweb repo (here). i was just about to create an issue about the problems/disagreements we had but it looks like @yyx990803 already came to same conclusions! basically im not a fan of these:
as you can see, Evan already suggested the last two and thats how he has implemented it in vite in fact. which means its already diverged from whats in this repo. about the wording... |
|
If splitting out
@rixo has been a huge HMR advocate in the Svelte ecosystem, I know they've integrated with esm-hmr and curious if they have any thoughts here re: these remaining differences. |
|
it seems like @yyx990803 went with: accept((mod) => { ... });
acceptDeps([a, b], ([a, b]) => { ... });which means you can't do both in one, as in you can't have "self" and some dependencies. im fine with that personally but is worth noting. as for your so maybe a middle ground is: accept((dep, meta) => { ... });
acceptDeps(['./foo', './bar'], ([foo, bar], meta) => { ... });or some such thing, since it seems like an edge case anyway for wanting this extra object. |
|
To me it's the "accept deps" use case that seems edgy, and the Apart from that, I kind of agree with @43081j that the name of the methods of the API could have more explicit names. Let's be honest, this naming is the legacy of Webpack that had its own approach at the time. The naming has been continued mimetically by other tools (Parcel, Nollup, Vite, my own confidential Rollup plugin...), each with their own adaptation on the approach... Now I'm personally hesitant to go back on this since it will break all existing Framework adapters (Svelte, React...), but I really don't have strong feelings about this. I'm going to follow suit, both in my adapter and my HMR provider, with anything that gathers a consensus in this regard. I want to insist heavily on the point that having even a "somewhat" standard API really pays off. For Svelte, I have basically the same code supporting Snowpack, Vite, and Rollup with just a tiny amount of care need to cater for the specific needs of each (Vite needing the accept call written in the module for static analysis, and Snowpack not passing the data object to the dispose handlers). In contrast, I need special adapter layers for Webpack and Nollup, and this undoubtedly adds maintenance burden and delays in adding support for this kind of targets. And so I think preserving, or even increasing, the consensus really is the important goal here. I'm willing to follow any direction regarding naming etc. in support of this goal, if some actor feels strongly enough that they'd would otherwise go their own way. I'd be pretty disappointed if the bubbled information, in one form of another, is dropped from the spec though. This really enables superior HMR behavior in the case of Svelte for example. |
|
Also worth noting, in addition to breaking all existing big Framework adapters (which is probably a pretty limited and closed-scope issue, because everyone concerned is closely following the game), changing namings would also impact every single user / lib that have made the effort of implementing HMR adapters in their own code. This seems like another level of breaking change, unfortunately. |
|
it would be interesting to hear from the rest of you what usage of the HMR API is like as opposed to automatic solutions. presumably vite "just works" and nobody has to deal with the hmr api directly, for example. our implementation is similar as we don't expect many to use the api directly but rather use plugins which interact with it. |
|
True that there is probably not a lot of end users that put their own hands into HMR. I personally do it because it can be useful to increase HMR comfort in a project, but that's probably because I have prior knowledge for it. However I suspect there exists a whole lot of users that have inherited some HMR code that they don't understand (i.e. can't fix) from a starter template, that is something that you typically never upgrade for a given project. (Still not a blocker to follow consensus, as far as I'm concerned.) |
|
FYI in modernweb repo we now adopted the same signatures as vite: accept((module) => { ... });
acceptDeps([dep1, dep2], ([dep1, dep2]) => { ... }); |
|
I was trying to use it, but it was always empty and when I tried to store data to persist during the module change, an exception occurred saying that it is just a getter. And in the guide it says that I could use it to pass data between module versions... |
Maybe you are not aware of it, but Vite has a working implementation of ESM-based HMR that covers most of the proposed usage. I think at the very least it would serve as a useful reference for this spec. Being included in the discussion also helps alignment of the API design to avoid potential fragmentation.
The text was updated successfully, but these errors were encountered: