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): revert early break from handleHotUpdate loop #5536

Merged

Conversation

dominikg
Copy link
Contributor

@dominikg dominikg commented Nov 3, 2021

reverts b3b8c61

Description

Breaking the loop that calls handleHotUpdate early once a plugin returns changes results in a "first plugin wins" situation that seems like an odd limitation.

Especially for plugins like windicss that maintain unrelated virtual modules based on the content of various files.
This means one file change event of single source file can result in multiple hmr updates from different plugins for completely unrelated modules, making it impossible for the user to find a plugin ordering that would work with the limitation mentioned above.

see windicss/vite-plugin-windicss#238 (comment) for details

Additional context

  • If multiple plugins return something from handleHotUpdate, should the changes be merged?
  • What about plugins calling ws.send directly?

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 Commit 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.

@dominikg dominikg changed the title fix(hmr): revert early break from handleHotUpdate loop introduced 2.5.5 fix(hmr): revert early break from handleHotUpdate loop introduced in 2.5.5 Nov 3, 2021
@Shinigami92
Copy link
Member

Should we add an inline comment, so the next one doesn't add it back? ^^'

@dominikg
Copy link
Contributor Author

dominikg commented Nov 3, 2021

i think git history should be sufficient to document and it may very well be the case that accomodating multiple updates from a single file change requires something entirely different to solve it amicably for all cases.

But if you have some comment in mind, add it. Some extra documentation on handleHotUpdate for plugin developers could help too.

@patak-js mentioned that the change was originally introduced to fix a problem in vitepress, but not sure what exactly.

@antfu antfu added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Nov 3, 2021
@yyx990803
Copy link
Member

I added this change because in vitepress there's a case where two plugins both expect to handle HMR and end up conflicting, but I agree this probably should be reverted (I have found a workaround in vitepress).

@patak-dev patak-dev changed the title fix(hmr): revert early break from handleHotUpdate loop introduced in 2.5.5 fix(hmr): revert early break from handleHotUpdate loop Nov 7, 2021
@patak-dev patak-dev merged commit 4abade6 into vitejs:main Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants