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(hmr): only invalidate lastHMRTimestamp of importers if the invalidated module is not a HMR boundary #13024

Merged
merged 6 commits into from Jun 21, 2023

Conversation

hyf0
Copy link
Contributor

@hyf0 hyf0 commented Apr 27, 2023

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?

  • 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 PR Title 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.

@stackblitz
Copy link

stackblitz bot commented Apr 27, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@hyf0 hyf0 marked this pull request as ready for review April 27, 2023 14:02
@patak-dev
Copy link
Member

Thanks a lot for porting the fix to Vite @hyf0 and @underfin! It is highly appreciated ❤️

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Apr 27, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ✅ success
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success
windicss ✅ success

Copy link
Member

@ArnaudBarre ArnaudBarre left a 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

@patak-dev patak-dev added this to the 4.4 milestone Apr 28, 2023
@hyf0 hyf0 changed the title fix(hmr): only invalidate importers if the invalidated module is not a HMR boundary fix(hmr): only invalidate lastHMRTimestamp of importers if the invalidated module is not a HMR boundary Apr 28, 2023
Copy link
Member

@ArnaudBarre ArnaudBarre left a 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!

@ArnaudBarre ArnaudBarre dismissed their stale review June 16, 2023 00:59

Updated in last commit

@patak-dev patak-dev merged commit 1143e0b into vitejs:main Jun 21, 2023
13 checks passed
xinxinhe1810 pushed a commit to xinxinhe1810/vite that referenced this pull request Jul 4, 2023
…lidated module is not a HMR boundary (vitejs#13024)

Co-authored-by: ArnaudBarre <arnaud.barre72@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HMR error: Cannot access '...' before initialization
5 participants