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

HMR should handle cyclic dependencies #10208

Closed
4 tasks done
zen0wu opened this issue Sep 23, 2022 · 10 comments · Fixed by #14867
Closed
4 tasks done

HMR should handle cyclic dependencies #10208

zen0wu opened this issue Sep 23, 2022 · 10 comments · Fixed by #14867

Comments

@zen0wu
Copy link

zen0wu commented Sep 23, 2022

Description

Previously in #2314, we've decided that HMR will not handle cyclic dependencies and instead reload the entire app, and we think that's the only way that it's correct.

However, I think we should reconsider that decision. Webpack apparently supports reloading app with circular dependencies, I haven't dig into the code, but webpack HMR works with our app with many many circular imports in it. so it's theoretically possible.

Suggested solution

Let's say I have 3 modules, A <-> B -> C,

  • If B is changed, we can clearly see that we only need to reload A + B, but leaving C
  • Theoretically, when a module is changed, it should only go up until all its transitive importers change, and whenever we hit a cycle, we should return instead of bail out the HMR.
  • This logic made me believe this is a simple change, but maybe I missed something obvious.

Alternative

No response

Additional context

No response

Validations

@bradleysimard
Copy link

We just migrated from CRA to ViteJS and after all the work required found that Vite's HMR is apparently not very good at handling cyclical dependencies when CRA handled it like a champ.

@alex-radulescu-b1
Copy link

We just migrated from CRA to ViteJS and after all the work required found that Vite's HMR is apparently not very good at handling cyclical dependencies when CRA handled it like a champ.

Same here. Vite is awesome, but some pieces for more complex apps still need a bit of work. Any chance of better circular dependency handling please?

@mahnunchik
Copy link

I'm looking forward for this feature.

1 similar comment
@MahoushoujoMadoka
Copy link

I'm looking forward for this feature.

@rkurbatov
Copy link
Contributor

@sztadii
Copy link

sztadii commented Oct 4, 2023

@yyx990803 @patak-dev @sapphi-red do you have any updates or thoughts about this issue? In the current project, I am using Vite, and I have never seen this cyclic problem in CRA / Webpack. It seems that only Vite has some issues with it. Could you please tell us if there is any work being done related to this issue?

@sztadii
Copy link

sztadii commented Oct 31, 2023

@bluwy any thoughts about this issue? I would love to see the fix for it 🙏

@bluwy
Copy link
Member

bluwy commented Nov 1, 2023

I looked into this issue a couple weeks ago actually, and got side-tracked into #14654, but I'll take a look at this again. I'm still not sure if it's completely safe and possible to do so, the previous fix to always reload didn't have tests to prevent a regression.

Anyways feel free to contribute a PR too if you like to see this fixed.

@zam157
Copy link

zam157 commented Nov 22, 2023

I think this issue should not to be closed: #14975 #15053

If Vite 5's handling of dependency cycles is just page reload, it will frustrate many people's confidence in migrating from webpack

@newives
Copy link

newives commented Dec 1, 2023

Completely agree, it is equivalent to saying that vite5 does not support hmr in circular dependencies, but it is normal operation to introduce modules into each other.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants