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): set isSelfAccepting unless it is delayed #8898

Merged
merged 1 commit into from
Jul 3, 2022

Conversation

sapphi-red
Copy link
Member

Description

In the following situation, a full reload should happen.

  • main.css depends on index.liquid by tailwind.
  • index.liquid's hmr is not handled by anything
  • edited index.liquid

The full reload was not happening because isSelfAccepting was undefined.

This PR changes isSelfAccepting to be set unless ensureEntryFromUrl is called from import-analysis.
The old behavior is to set isSelfAccepting if isHTMLRequest(url) || canSkipImportAnalysis(url) is true.

refs #7561 #7895 #7898

Additional context

Found while checking #8890.


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.

@sapphi-red sapphi-red added feat: hmr p3-minor-bug An edge case that only affects very specific usage (priority) labels Jul 3, 2022
Comment on lines +175 to +178
if (updates.length === 0) {
debugHmr(colors.yellow(`no update happened `) + colors.dim(file))
return
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When updates was [] the config.logger.info below was outputing 22:42:33 [vite] .

@netlify
Copy link

netlify bot commented Jul 3, 2022

Deploy Preview for vite-docs-main ready!

Name Link
🔨 Latest commit 946ebb0
🔍 Latest deploy log https://app.netlify.com/sites/vite-docs-main/deploys/62c19c5edffbfe000838b609
😎 Deploy Preview https://deploy-preview-8898--vite-docs-main.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@patak-dev
Copy link
Member

This is a lot safer, thanks Sapphi. @brillout, you may also want to review in case there is a regression here.

@patak-dev patak-dev merged commit ae34565 into vitejs:main Jul 3, 2022
@sapphi-red sapphi-red deleted the fix/hmr-self-accepting branch July 3, 2022 16:25
@brillout
Copy link
Contributor

brillout commented Jul 4, 2022

Looks good 👍. I'll test it on vite-plugin-ssr's HMR test suite as soon as it's released.

@patak-dev
Copy link
Member

Released as v3.0.0-beta.6 @brillout

@brillout
Copy link
Contributor

brillout commented Jul 4, 2022

HMR tests are still green with beta.6.

@aleclarson
Copy link
Member

aleclarson commented Nov 16, 2022

I really don't like the naming used here.. 😬

As I understand it, we are setting isSelfAccepting to false in the ModuleNode constructor in order to allow HMR update propagation, because propagation is prevented when isSelfAccepting is undefined. Is that right?

If so, maybe allowHmrPropagation would be a better name for this argument (instead of setIsSelfAccepting). Although, it seems a little hacky to be overloading isSelfAccepting === undefined to mean "prevent HMR propagation".

@@ -356,7 +356,12 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
// its last updated timestamp to force the browser to fetch the most
// up-to-date version of this module.
try {
const depModule = await moduleGraph.ensureEntryFromUrl(url, ssr)
// delay setting `isSelfAccepting` until the file is actually used (#7870)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is a little confusing. We should move it closer to what it's referencing (above the canSkipImportAnalysis call) and explain why it's necessary.

@patak-dev
Copy link
Member

I think allowHmrPropagation would also be confusing here. Maybe delayHmrPropagation? The gist of the issue in vite-plugin-ssr that @brillout found was that HMR was being triggered for modules that haven't yet been loaded by the browser (so making isSelfAccepting undefined until we know its value, that is correct independent of this issue because if not we are guessing, also fixed his issue).

Maybe the parameter should be called isSelfAccepting and by default it is undefined, meaning that when we are creating the module, we don't yet know (we need to do import analysis to find out). And if you pass true or false, it means you do know the proper value at creation time. I can do a PR to change it to this if you think it is better @sapphi-red @aleclarson

@aleclarson
Copy link
Member

Maybe the parameter should be called isSelfAccepting and by default it is undefined, meaning that when we are creating the module, we don't yet know (we need to do import analysis to find out). And if you pass true or false, it means you do know the proper value at creation time.

👍

@brillout
Copy link
Contributor

I think allowHmrPropagation would also be confusing here. Maybe delayHmrPropagation? The gist of the issue in vite-plugin-ssr that @brillout found was that HMR was being triggered for modules that haven't yet been loaded by the browser (so making isSelfAccepting undefined until we know its value, that is correct independent of this issue because if not we are guessing, also fixed his issue).

Exactly.

@patak-dev
Copy link
Member

@aleclarson PR refactoring from setIsSelfAccepted to { isSelfAccepted: boolean | undefined } here:

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.

4 participants