self accepting HMR modules should not invalidate the whole import chain (?)

[AlexandreBonaventure]

Describe the bug

We unravelled a bug recently in our application with HMR. When we change a single .vue file it throws an error like this one:

This sounds like a circular dependency bug so we started trying to find it... but after some investigation we noticed this error message only shows up in vite@2.0.0-beta.34 and later.
This commit is the one we are looking at:
ca8442c

Here's are two videos:

1. showing what's happening on 2.0.0-beta.33: everything works correctly, the change is update live and does not throw any error
   https://user-images.githubusercontent.com/4596409/121266195-12b6a800-c888-11eb-9724-20c996aa8e12.mov

2. showing what's happening on 2.0.0-beta.34: here the whole importer chain is invalidated.
   https://user-images.githubusercontent.com/4596409/121266365-63c69c00-c888-11eb-8c4d-c35fda601514.mov

Not only it throws an error, but it seems inefficient to invalidate the whole chain just to propagate one change on a component.

We didn't notice the bug until today despite running latest vite (2.3.6) because the error is thrown only on this component. I guess because it has a specific set of importers/deps that are not playing well with this whole invalidation process.

Thanks

Reproduction

Right now, this is very difficult to provide a minimal repro, because the bug is probably only showing up at scale. Waiting for your feedback before doing anything.

System Info

Not relevant I think

Logs

---

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

• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Provide a description in this issue that describes the bug.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/vue-next instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.

[anncwb]

I have also encountered it, which is very troublesome in large projects. I have been looking for a solution to this problem.

[jrmyio]

Also ran in this just now. In a large project I have quite some circular references between state related files.

Any circular reference between the files are actually tackled by utilizing an ./internal file from where all children import their dependencies (as described by mwestrate in https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de )

However, with vite, changing any of these files creates a full refresh, with the result that hundreds (if not thousands) of http calls will be triggered. I only started experimenting with vite as of 2 days ago but this is a real showstopper for me.

For now I monkey patched ca8442c#diff-863416a9b0769ffb1d7e36b6840ce6d0d84170a80a5c4ce4539464b0ebcbd137R183 to return false and then things work smoothly.

If required I could set up a codesample.

[AlexandreBonaventure]

After digging more, we tried to remove every circular dependencies in our project thanks to this tool: https://github.com/acrazing/dpdm

After removing circular dependencies it did fix the error (reiterating but before 2.0.0-beta.33 it worked just fine) BUT it is still re-importing the whole chain which is pretty huge in a medium-scale project. Here's a screenshot when I just change a copy in a route component (taking ~700ms per update):

Moreover, in order to remove every circular dependencies, we had to hack around because sometimes cyclic dependencies are an actual legit use-case. So I'd like to keep it at a few places.

I tried to provide a minimal reproduction here:
https://github.com/AlexandreBonaventure/vite-circ-deps-hmr
But unfortunately, the project is too small to highlight any sort of problem here.

I guess the issue here is similar to #2314, but when Evan says:

This is expected behavior. If there are circular imports, the only way to ensure correctness is to invalidate all modules in the chain. If you want to avoid it, consider refactor your code to avoid circular imports.

it is not entirely true, since the problem persists even when removing cyclic deps.

Maybe it's worth considering analysing the imports map and cyclic dependencies previous to HMR update, in order to be smarter when refreshing components.

[gustavopch]

In my case, I like to structure my project as:

lib/
  algolia.js
  firebase.js
  index.js

Where index.js contains:

export * as Algolia from './algolia.js'
export * as Firebase from './firebase.js'

So that I can have all my stuff under a single import that exposes everything through namespaces. By doing that, I never have to even think about dealing with conflicting imports like these:

import { get } from '$lib/algolia'
import { get } from '$lib/firebase' // Ops, name conflict

Instead, it's just:

import * as Lib from '$lib'

Lib.Algolia.get()
Lib.Firebase.get()

IMO, it's far superior to aliasing imports or ensuring modules use different names for their exports, but, unfortunately, it does cause problems with Vite's HMR as described in this issue because it ends up having lots of cyclic dependencies. Apart from this, those cyclic dependencies are harmless. I'm hoping I can find some workaround so I don't have to stop using namespaces as I do.

[gustavopch]

OK, it turns out I was able to solve my specific case by writing a local Vite plugin that expands those namespace imports. So, basically, it turns this:

import * as Lib from '$lib'

Lib.Algolia.get()
Lib.Firebase.get()

into this:

import * as Lib_Algolia from '$lib/algolia.js'
import * as Lib_Algolia from '$lib/firebase.js'

Lib_Algolia.get()
Lib_Firebase.get()

That way, as my cyclic dependencies happen just because of the way I use namespaces, they're mostly eliminated.

[github-actions]

Hello @AlexandreBonaventure. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with need reproduction will be closed if they have no activity within 3 days.

[bluwy]

Hi, we need a reproduction to look into an issue. It looks like it requires a specific directory configuration to reproduce it. Having a repro would help others easily debug the issue in Vite.

[AlexandreBonaventure]

This issue is opened since July 2021, like I said above:

Right now, this is very difficult to provide a minimal repro, because the bug is probably only showing up at scale. Waiting for your feedback before doing anything.

I'll be glad to help solving the problem, but I need more time, I can't realistically create a reproduction within 3 days...

[patak-dev]

Hey @AlexandreBonaventure, there is no need to rush. If you manage to create a reproduction please open a new issue against the latest versions. In the meantime, I think we can keep this issue closed. Thanks!

[sebastienbarre]

@patak-dev I'm not sure if this is the right place to ask this, but is there a document that clarifies how and when dependencies are reloaded in Vite in the context of HMR? This would greatly help.

I'm trying to port our sizable app from Snowpack to Vite and the results are puzzling in dev mode. I'm not sure exactly why yet, but HMR is much slower in Vite. Each time I update a core component in my app (say, <Button>, or <Badge>) this triggers a deluge of dependencies being reloaded, including modules corresponding to components that are not even mounted on screen. This was not the case with Snowpack, where refreshing was nearly instant, no matter which component.

For example:

import { AppCacheBuster } from './App/AppCacheBuster';
import { App } from './App/App';
...

ReactDOM.render(
  <>
    {import.meta.env.PROD && <AppCacheBuster />}
    <App />
  </>,
  document.getElementById('root'),
);

In this example, AppCacheBuster.jsx does have an import { Button } from '@component/Element/Button';, but it is never used/mounted in development mode. I do not quite understand why AppCacheBuster.jsx would need to be hot-updated here. Our app has routes that are lazy-loaded and these appear to be hot-reloaded as well if they happen to use <Button>, even though I had not navigated to them yet. Now I have tried to reproduce this in a small example, and I couldn't. Given the popularity of Vite, I'm going to assume I'm doing something wrong, so I'm wondering if there are other limitations I'm not understanding that we may have introduced in our (large) codebase, limitations that could be documented somewhere?

Thank you