fix: fix HMR propagation when imports not analyzed

[brillout]

Description

In some situations, the HMR signal hits a module that has not been analyzed.

For example, a dynamic import that is never called const neverCalled = () => import('./dep.js'). Here the dep.js file is never loaded and thus never analyzed by the importAnalysis.ts plugin.

If the HMR signal hits such non-analyzed module, then this means that the module has not been loaded.

Because it has not be loaeded there is nothing to refresh; we can and should stop the HMR propagation. Otherwise the HMR signal triggers a wrongful full page reload.

Additional context

This is a vps 0.4 release blocker. (This situation used to be an edge case with vps 0.3 but, with the new vps architecture, this is a common and problematic case.)

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[patak-dev]

Interesting problem. The fix isn't perfect because after reloading the client page, if the module was loaded before, then the HMR issue will still appear.

It also works because we are only eagerly traversing static imports here

vite/packages/vite/src/node/plugins/importAnalysis.ts

Line 655 in c296130

// pre-transform known direct imports

But dynamic imports could potentially be included at one point if the server is idle, breaking this fix.

Would you explain a bit more about how this issue appears in vps? We could still merge it, but feels a bit writtle. I wonder if there is something better we could do here. It shouldn't be rare for your users to experience a page reload after the server starts, and then this patch is gone.

[patak-dev]

Interesting problem. The fix isn't perfect because after reloading the client page, if the module was loaded before, then the HMR issue will still appear.

It also works because we are only eagerly traversing static imports here

vite/packages/vite/src/node/plugins/importAnalysis.ts

Line 655 in c296130

// pre-transform known direct imports

But dynamic imports could potentially be included at one point if the server is idle, breaking this fix.

Would you explain a bit more about how this issue appears in vps? We could still merge it, but feels a bit writtle. I wonder if there is something better we could do here. It shouldn't be rare for your users to experience a page reload after the server starts, and then this patch is gone.

[brillout]

The original problem, for which I wrote this fix, is when the dyamically imported module defines import.meta.hot.accept.

In that case, the fix is reliable.

Would you explain a bit more about how this issue appears in vps?

vps 0.4 has a custom modifier ?extractExportNames and has a import.meta.importGlob('/**/*.page.js', { query: 'extractExportNames' }) (using vite-plugin-glob). These .page.js?extractExportNames are loaded only if needed. Prior to this PR, Vite assumes that isSelfAccepting is false if .page.js?extractExportNames has not been loaded, leading to a full page reload. This essentially breaks HMR for vps. The PR fixes it reliably because as soon as .page.js?extractExportNames is analyzed, Vite knows that the module has import.meta.hot.accept and then HMR works correctly (even if the module is not loaded).

You're right - and I didn't think of that - the fix is not reliable for dynamically imported modules that don't have import.meta.hot.accept. I'm trying to find a solution but I can't find one. It seems hard to achieve since the question whether the module is loaded is resolved dynamically at runtime, while the importAnlysis.ts plugin works by statically analysing the code. I think the current approach of just assuming that the module is loaded and propagating HMR accordingly is an acceptable situation. So far, I can't think of a solution that wouldn't require profound changes.

My proposal: I'll update the PR to add a link to our discussion (I'll also update isSelfAccepting?: boolean) and then we merge, if you believe it's ok.

[patak-dev]

The original problem, for which I wrote this fix, is when the dyamically imported module defines import.meta.hot.accept.

In that case, the fix is reliable.

Ah, it is true, if accept is defined then once it is loaded this fix will improve HMR even after reloads 👍🏼

I'm also trying to think of a way of making the other case work, but it requires communication between each client and the server and keep also track of sessions for each client.

I think we should merge this PR after the code review, at least it is an approval from me. Let's try to get other team members to check it out before though (we are no longer in the beta, so we need to be very careful until we open that window again)

[brillout]

PR updated & ready for review.

[brillout]

Thanks for the commits. Looking forward to (help folks) build Vite/vps frameworks.

[mazerty]

hi :)
i'm sorry but this fix seems to prevent hmr to work with my virtual module https://www.npmjs.com/package/@__mazerty__/rollup-plugin-pages
it reads md(x) files to create a frontmatter/metadata array that can be imported
i tried to implement handleHotUpdate on it, but it doesn't work if the virtual module has been imported but the actual md(x) file hasn't yet
when debugging with vite --debug I get the message vite:hmr [propagate update] stop propagation because not analyzed and so my handleHotUpdate isn't called :(

i'm quite new to vite though, so maybe i'm doing something wrong 🤪

[bluwy]

@mazerty perhaps you need to call this.addWatchFile for each MDX files you're loading inside the load hook so that Vite is able to know what that module depends on for HMR to (maybe) work. Other than that, I'm not sure if this can be fixed. I'd also suggest getting support at https://chat.vitejs.dev or on GitHub discussion.

[mazerty]

thanks a lot for your reply :)
i tried using this.addWatchFile and the mdx file now appears in the _addedImports part of the context, but i still get the vite:hmr [propagate update] stop propagation because not analyzed message and it doesn't get refreshed :(
i don't have the time right now to dive more into this, but when i do i'll ask for help on discord :)