-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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(hmr): only invalidate lastHMRTimestamp
of importers if the invalidated module is not a HMR boundary
#13024
Conversation
|
/ecosystem-ci run |
This comment was marked as outdated.
This comment was marked as outdated.
/ecosystem-ci run |
📝 Ran ecosystem CI: Open
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into this! There is effectively something we can do here to be smarter on invalidation. But the current changes creates two issues I think:
- If a module is a dead end, it's not invalidated
- When a dead end is found, we continue to search for boundaries which can slow down full page reload in case of big module graph
If you want I can try to look deeper on this later this weekend
Edit: I'm less sure for the second point. Because actually that can be beneficial to avoid more invalidation if other branches contains HMR boundaries. I think we need to rethink this code further to not be invalidate then search for boundaries or the opposite, but explore the graph to do both at the same time. Easier said than done
lastHMRTimestamp
of importers if the invalidated module is not a HMR boundary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I took a deeper dive and I discovered (thanks to @patak-dev) that handling circular imports & tailwind has already created a lot of complexity and invalidate way too many things.
For example when a file change, we first walk through the module graph to invalidate, but with isHMR
to false
which doesn't update lastHMRTimestamp
and so doesn't trigger this bug.
I will see how to improve that once I've finished my work to integrate Lightning CSS.
In the meantime I'm ok with this PR if we move the invalidate call between the propagateUpdate
call and hasDeadEnd check.
Thanks again for helping fix that!
…lidated module is not a HMR boundary (vitejs#13024) Co-authored-by: ArnaudBarre <[email protected]>
Description
Try to fix #3033 while pair programming with @underfin.
Additional context
We are fixing some react HMR issues in Rspack and found that Vite has a similar issue. After web-infra-dev/rspack#2714, I start to investigate the issue of Vite, which turns out the issue has a different cause than Rspack. After a hard coding with @underfin, we locate the issue, which is caused by cycle dependency.
We came out of this fix, not sure it breaks other things.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).