feat(hmr): reload for circular imports only if error

[bluwy]

Description

Because people love circular imports.

This PR will not forcefully reload the page if a circular import is detected for the HMR boundary. Instead, it'll signal to the client to "watch out" for fails and if so, trigger a reload.

Currently it'll only "watch out" for fails when dynamically importing the new module, I think that's the only place we need to catch. Errors from HMR handlers don't need to be caught I think.

Additional context

---

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, especially the Pull Request Guidelines.
• 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).
• Update the corresponding documentation if needed.
• Ideally, include relevant tests that fail without this PR but pass with it.

[skovhus]

I'm working on a codebase (https://linear.app/) with many inherent circular dependencies that will take a lot of effort to remove. So I'm very curious about improving the HMR experience.

@bluwy what is the status of this PR?

Any downsides to this approach @patak-dev?

[patak-dev]

We're discussing this one with the team. I'm good merging this one to try it out in the wild, and we can keep improving warning messages to guide users. I'd like @ArnaudBarre's opinion here, as he has been dealing with many issues due to circular imports and HMR. One option would be to offer this feature under a (experimental?) flag so we can get it out and test things out before deciding if this should be the default

[skovhus]

One option would be to offer this feature under a (experimental?) flag so we can get it out and test things out before deciding if this should be the default

I think that would be a good start. I'm happy to provide feedback and test it out.

[skovhus]

What's the recommended way if I just want to test out this PR on our codebase? Do I need to use pnpm patch or is there a simpler way to do this?

[patak-dev]

What's the recommended way if I just want to test out this PR on our codebase? Do I need to use pnpm patch or is there a simpler way to do this?

One way is to link vite, as described here. I like to use overrides, though. If you are using pnpm, you can add an override in your package.json:

  "pnpm": {
    "overrides": {
      "vite": "/path-to-vite-root/packages/vite"
    }
  }

Other package manager have similar features (npm, yarn)

[ArnaudBarre]

Looks good!
I think HasDeadEnd can be cleaned up

I think we could send back a message to the server saying that this file caused reload (like with invalidate) so that even without hmr debug on you can know the reason behind the full page reload

[bluwy]

I think we could send back a message to the server saying that this file caused reload (like with invalidate) so that even without hmr debug on you can know the reason behind the full page reload

It's usually tricky to have the client send back to the server like that since you could have multiple clients connected at the same time. We could have the hmr update message with the (circular imports) still though but I didn't implement that in this PR. I figured https://vitejs.dev/guide/troubleshooting.html#a-full-reload-happens-instead-of-hmr would be sufficient.

One option would be to offer this feature under a (experimental?) flag so we can get it out and test things out before deciding if this should be the default

I think we could fit this in 5.1 without an option. Without a page reload, it reverts to the Vite 4 behaviour so I think it's safe, but the catch handler would work additionally to detect circular fails and reloads instead.

[sapphi-red]

I guess there are some false-positives/false-negatives with the way this PR detects whether a reload is needed or not. But I think the DX improvement justifies having them.

[ArnaudBarre]

Because of the default of reload often clear logs, I think use console.log instead of debug is fine so people can quicly see it before the reload and enable "preserve logs" afterwards to read it.

But maybe a lot of devs already uses "preserve logs" and it will be annoying

[bluwy]

I think most browsers will default to show debug logs, so it shouldn't get hidden in most case unless they explicitly hide it. The vite connection messages are also debug so currently leaning towards the consistency.

[sapphi-red]

I think most browsers will default to show debug logs, so it shouldn't get hidden in most case unless they explicitly hide it.

At least for Chrome, console.debug is hidden by default. (console.info/warn/error isn't hidden by default)

[skovhus]

FYI I tested this out. Didn't really see any improvement in our case, but also no regression. Which cases would a circular dependency be flagged but not fail the client?

[bluwy]

If you're accessing the exported names within a circular dependency loop on initialization, it could trigger an error. If they're lazily referenced, usually it will work. If you did hit the issue, perhaps you're seeing #3033. The fix for it (#13024) IMO wasn't safe, that a single module could have duplicated instances to workaround the issue. Today, that issue is fixed by triggering a full reload instead.

At least for Chrome, console.debug is hidden by default. (console.info/warn/error isn't hidden by default)

If y'all prefer a different kind of log, I don't mind changing it too 👍 Maybe console.info makes more sense here.

[sapphi-red]

I don't have a strong opinion but if I had to choose, I'll go with console.info.

[AlexanderFarkas]

Is that possible to opt out of this behaviour? So page refresh is triggered when circular import detected.
On v5.1.6 now I don't have page reload at all, if changed code has circular imports - so I have to reload page manually

[bluwy]

Is there a reason you want a page reload? HMR should kick in instead so you'd see the changes. If HMR didn't kick in and it didn't cause any errors for us to catch, then that would be a bug in the HMR implementation I think.

[AlexanderFarkas]

HMR kicks in, I see updated version in the Network tab of the browser, logs in the console, but nothing changes until I force refresh the whole page.

That happens only on a page with circular imports and inconsistent exports. So, if I export both MobX class and a component, it just reports error "hmr invalidate" to the console.

So I believe it's expected behaviour. I had to downgrade to 5.0.12 to get rid of this.

[bluwy]

I think it would be best if you can open a new issue for it with a repro of the problem. Depending if this is used for react/vue/preact etc, it might be a Vite plugin specific issue. Otherwise I don't think there should be a way to opt of of this PR, there's a bigger problem if HMR fails this way.

[ricardo17coelho]

@AlexanderFarkas if u open a new issue, pls put it here.. i'm also facing this issue.
I fixed all the circular imports with help from this plugin: https://github.com/threedayAAAAA/vite-plugin-circular-dependency
but still have the issue on the v. 5.1.6 but works on 5.0.12 like in your case.

[AlexanderFarkas]

I am working on reproduction sample, but no luck with if project is new. Once I manage to reproduce, I will create the issue.