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: make addWatchFile() work (fix #7024) #9723

Merged
merged 6 commits into from
Nov 16, 2022

Conversation

zqianem
Copy link
Contributor

@zqianem zqianem commented Aug 18, 2022

Description

fixes #7024 by ensuring addWatchFile adds files to the module graph

Additional context

The intention of db4ba56 was to allow plugins to add files to the module graph via addWatchFile() so that changes to those files would cause HMR updates. However, for files without any ES imports, the import analysis returns early and the files passed to addWatchFile() in this._addedImports are never added to the graph. Checking for the existence of this._addedImports fixes this.

Additionally, the new forceSkipImportAnalysis flag is needed to mark the newly created modules as not needing additional analysis so that the HMR update can propogate (see #8898).

I could try to add the tests from #4461 if they are still relevant. Cherry-picked the test from #4461 and verified that it fails without 33938a3.


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.

zqianem added a commit to byrd-polar/fluid-earth that referenced this pull request Aug 23, 2022
Now edits to GLSL files will cause dev server to automatically update
the page, as expected. Previously only worked for the GLSL files
directly imported by JS.

Done by patching dependencies; should try to get some version of these
changes merged upstream.

See vitejs/vite#9723
@zqianem zqianem changed the title fix(dev): make addWatchFile() work fix: make addWatchFile() work (fix #7024) Sep 22, 2022
@zqianem
Copy link
Contributor Author

zqianem commented Sep 22, 2022

Rebased onto latest main and re-ordered the commits (add tests first) for easier verification of fix.

@zqianem
Copy link
Contributor Author

zqianem commented Nov 16, 2022

Rebased again to verify that the fix is still necessary (the added test fails without) and that it still works. I think the failed tests are due to flakiness — different tests fail on the same commit in my fork.

@patak-dev could you please take a look at this? I'm worried that this PR has missed the triage process due to lack of labels and wasn't sure how to reach out; apologies if this is already on the radar. Thanks!

@patak-dev patak-dev added p3-minor-bug An edge case that only affects very specific usage (priority) feat: hmr labels Nov 16, 2022
@patak-dev
Copy link
Member

@zqianem thanks for the ping (and for the patience). Looks good to me 👍🏼
I will send it to #contributing to https://chat.vitejs.dev to get another review or merge it tomorrow if not because we can test it safely during the beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addWatchFile can not be used to watch files outside of the module graph
4 participants