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

fix: fix HMR propagation when imports not analyzed #7561

Merged
merged 4 commits into from Apr 4, 2022

Conversation

brillout
Copy link
Contributor

@brillout brillout commented Apr 1, 2022

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
Copy link
Member

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

// 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.

1 similar comment
@patak-dev
Copy link
Member

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

// 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
Copy link
Contributor Author

brillout commented Apr 1, 2022

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
Copy link
Member

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)

@patak-dev patak-dev added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) feat: hmr labels Apr 1, 2022
@brillout
Copy link
Contributor Author

brillout commented Apr 1, 2022

PR updated & ready for review.

@brillout
Copy link
Contributor Author

brillout commented Apr 2, 2022

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

@mazerty
Copy link

mazerty commented Nov 26, 2023

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
Copy link
Member

bluwy commented Nov 27, 2023

@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
Copy link

mazerty commented Nov 28, 2023

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants