Fix HMR Regression

[aaronjensen]

#1620

Changes

It fixes a regression introduced in c5490fd

Copied from the discussion:

If I have a module tree like this:

App imports Guide

Guide imports Bubble

Bubble exports a string const

Guide uses the string in a React compnoent

If I change Bubble, then HMR works and I see the new Bubble
If I change Guide AFTER that, then the previous version of Bubble is used, which regresses the first HMR.

Here's a video:

Screen Recording 2020-11-16 at 9.05.19 PM.mp4.zip

Here's a repro:

https://github.com/aaronjensen/snowpack-hmr-repro

Testing

Manually, using the repro

Docs

No, it's a bug fix.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/4h2feuvcf
✅ Preview: https://snowpack-git-fix-hmr-regression.pikapkg.vercel.app

[aaronjensen]

This apparently causes dependencies of the hot reloaded module to reload when they should not, so this probably isn't the right fix. I'm still unclear what would cause the old child module to be used in the initial repro, so hopefully someone can shed some light on that and suggest a better fix.

/cc @Akimyou

[aaronjensen]

After doing some more digging on the original, it looks like what's happening is that when the child module changes and it bubbles to the parent module which hot accepts, that parent module gets requested with an mtime. That child module is imported within the parent module with an mtime as well, though it is unclear what actually adds that mtime. I don't see it anywhere in the snowpack source, but I could be missing it.

When the parent module changes on its own, the mtime on the import for the parent module is updated, but the mtime on the import for the child within the parent module is reset to not exist, causing the originally imported version to take precedence.

So, whatever is adding the mtime to the imports within the built file needs to remember the latest mtime for each file and not reset it.

Any pointers on what repo this code is in?

[aaronjensen]

@FredKSchott Could I please get a pointer here? I've spent hours looking into this and I cannot figure out what causes import statements to be rewritten with the mtime on files served from the dev server. If I can find that, I might be able to figure out how to ensure that those mtimes persist.

[aaronjensen]

@FredKSchott Okay, I tracked it down. This fixes it and is hopefully acceptable. We track the mtime on the hot entry so that we can reuse it anytime a module that depends on a module that has previously changed is changed.

[FredKSchott]

@aaronjensen thanks for digging into this and spending the time to fix! I've been away from open source for the last week for the holidays, but just got back and can take a look at this sometime in the next day or so as I get caught up.

[aaronjensen]

Great, thank you. I hope you enjoyed your holiday!

[FredKSchott]

so this is a tricky part of ESM HMR, since you don't actually control the module registry and can't swap modules in and out. To get around this limitation, modules behave differently based on whether isHmrAccepted is true or false:

• true: This module accepts HMR updates without triggering a full page reload. That means that the accept handler "accepts and applies" updates to the current module instance. New versions are loaded via ?mtime, but they aren't replaced into are folded into the existing application via the accept handler. The original URL is the only one that exists in the application.
• false: This is a normal module. When it needsReplacement, a new version is loaded into the application. Multiple are loaded within an HMR-enabled session, but only the most recent one should be imported and referenced within the application.

(tldr) All of that context is just to say: this fix should only apply if isHmrAccepted = false. If isHmrAccepted = true, then the current behavior is expected.

[aaronjensen]

Okay, I don't totally understand the context, but I think I got the logic right. Can you give that a look when you get a chance, please? Thanks!

[vercel]

@aaronjensen is attempting to deploy a commit to the Pika Team on Vercel.

A member of the Team first needs to authorize it.

[aaronjensen]

@FredKSchott is this one good now?

[FredKSchott]

Yes! Thanks for your patience, had a busy week getting the Snowpack v3 RC out, merging this now and may do a bit of polish on top of it, but overall this LGTM

[aaronjensen]

Great, thank you, and congrats on the v3 RC.

[aaronjensen]

@FredKSchott I don't think this ever made it into Snowpack as a fix. 2.18.1 seems broken, potentially due to the changes in 56cb53d and it's still just as broken in 3.X. I've opened a new issue #3245. Is there anything I can do to help get it fixed?