HMR fixes #2461
HMR fixes #2461
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/kyrwlqreo |
| ...deps.map((d) => import(d + `?mtime=${updateID}`)), | ||
| import(id + `?mtime=${updateId}`), | ||
| // FIXME we can't know the correct updateId for dependencies at this point | ||
| ...deps.map((d) => import(d + `?mtime=${updateId}`)), |
There was a problem hiding this comment.
This is still a problem that needs solving... At this point, we have no guarantee that the updateId we use here matches the one known by the server (that would be used in subsequent import rewriting, and could result in duplicated version of the dep).
| @@ -1376,7 +1412,12 @@ export async function startServer(commandOptions: CommandOptions): Promise<Snowp | |||
| getServerRuntime: (options) => getServerRuntime(sp, options), | |||
| async shutdown() { | |||
| await watcher.close(); | |||
| server.close(); | |||
| await new Promise<void>((resolve, reject) => { | |||
There was a problem hiding this comment.
Added this to help the test runner really knowing when everything is closed, and avoid hanging indefinitely when done... I must confess, this hasn't proven a massive success yet (the child process running the test currently has to forcefully exit to avoid hanging -- main suspect is Chokidar's FS watchers). Yet, I believe this is still cleaner to report the async completion, as is made in this change.
|
@rixo sorry for letting this sit, is this still relevant? Happy to help get it merged if so. |
|
@FredKSchott Not sure. I'd need to rewire my tests to the last version of Snowpack, but I'm a bit of time compressed these days :-/ |
|
I'm running into the issues listed in the top comment too. I was about to submit a PR before I saw this one. The changes I made are roughly the same as those in this PR but are built off of |
|
Hi @rixo , @kevinbarabash , I assume there was no other pull request that fixes those issues, right? So this one is still valid and open for merging? |
Opening a draft PR to share current progress and synchronize efforts...
Changes
Current HMR engine has a few bugs... The gist of the problems I've identified are duplicated copies of the same module in the browser (as I detailed in #2238, that triggered this investigation), or HMR pulling back outdated versions of some modules, when some of their consuming modules change.
I believe the following current issues, raised in discussions, are all related to these bugs and would be fixed by this PR (the first one is mine, mentioned above, so I'm sure):
The proposed fix in this PR is:
assign
updateId(used as the?mtime=parameter) on the server side, and pass them to the client, instead of generating them client-side when receiving an updatethis allows to always rewrite imports in JS files to use the last known
updateId, to avoid importing outdated modules on subsequent updates of importersensure that all the modules impacted by a HMR update bubble gets the same
updateId, to avoid that a module impacted by multiple paths (transitive unaccepted dependencies) may get differentupdateId(and so the browser will incorrectly fetch different copies of the same module version)The PR currently focuses on HMR with
.jsmodules. Not too sure how CSS would/should be impacted yet; any suggestion welcome on this aspect.I think there are other related points that would deserve being discussed, but they're not directly the object of this PR (yet?), so I'll detail them in comments later.
Testing
I've setup a test project to help corner & fix the aforementioned issues, all while trying to avoid creating new ones: https://github.com/rixo/snowgun
Current Snowpack (3.0.11) fails 3/5 test cases... which is a bit expected since, for now, I've mainly written tests that illustrates the problems I had already detected.
The branch of this PR passes all current test cases.
To run the tests with [email protected]:
I'm planning on adding more test cases to further specify and guarantee HMR behaviour (thinking about dynamic imports, and what happens when multiple tabs are opened, notably). But that's a bit time consuming, so that may be kind of a long haul effort. Also, I'd like your feedback on the approach, before committing too far into it...
Docs
Only bug fixes for now.