fix(hmr): set isSelfAccepting unless it is delayed by sapphi-red · Pull Request #8898 · vitejs/vite

sapphi-red · 2022-07-03T13:40:43Z

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 · 2022-07-03T13:42:40Z

packages/vite/src/node/server/hmr.ts

+  if (updates.length === 0) {
+    debugHmr(colors.yellow(`no update happened `) + colors.dim(file))
+    return
+  }


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

netlify · 2022-07-03T13:44:10Z

✅ 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... Use your smartphone camera to open QR code link.

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

patak-cat · 2022-07-03T15:06:04Z

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

brillout · 2022-07-04T07:51:33Z

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

patak-cat · 2022-07-04T08:21:53Z

Released as v3.0.0-beta.6 @brillout

brillout · 2022-07-04T16:02:04Z

HMR tests are still green with beta.6.

aleclarson · 2022-11-16T04:28:57Z

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

aleclarson · 2022-11-16T04:38:28Z

packages/vite/src/node/plugins/importAnalysis.ts

          // 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)


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-cat · 2022-11-16T07:59:42Z

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 · 2022-11-16T15:48:03Z

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 · 2022-11-16T16:04:21Z

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-cat · 2022-11-16T22:55:14Z

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

chore: setIsSelfAccepting param to isSelfAccepting #10960

fix(hmr): set isSelfAccepting unless it is delayed

946ebb0

sapphi-red added feat: hmr p3-minor-bug An edge case that only affects very specific usage (priority) labels Jul 3, 2022

sapphi-red commented Jul 3, 2022

View reviewed changes

patak-cat approved these changes Jul 3, 2022

View reviewed changes

patak-cat merged commit ae34565 into vitejs:main Jul 3, 2022

sapphi-red deleted the fix/hmr-self-accepting branch July 3, 2022 16:25

zqianem mentioned this pull request Aug 18, 2022

fix: make addWatchFile() work (fix #7024) #9723

Merged

9 tasks

tuchg mentioned this pull request Oct 23, 2022

fix(hmr): cannot reload after missing import on server startup (#9534) #10602

Merged

9 tasks

aleclarson reviewed Nov 16, 2022

View reviewed changes

patak-cat mentioned this pull request Nov 16, 2022

chore: setIsSelfAccepting param to isSelfAccepting #10960

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(hmr): set isSelfAccepting unless it is delayed#8898

fix(hmr): set isSelfAccepting unless it is delayed#8898
patak-cat merged 1 commit intovitejs:mainfrom
sapphi-red:fix/hmr-self-accepting

sapphi-red commented Jul 3, 2022

Uh oh!

sapphi-red Jul 3, 2022

Uh oh!

netlify bot commented Jul 3, 2022 •

edited

Loading

Uh oh!

patak-cat commented Jul 3, 2022

Uh oh!

brillout commented Jul 4, 2022

Uh oh!

patak-cat commented Jul 4, 2022

Uh oh!

brillout commented Jul 4, 2022

Uh oh!

aleclarson commented Nov 16, 2022 •

edited

Loading

Uh oh!

aleclarson Nov 16, 2022

Uh oh!

patak-cat commented Nov 16, 2022

Uh oh!

aleclarson commented Nov 16, 2022

Uh oh!

brillout commented Nov 16, 2022

Uh oh!

patak-cat commented Nov 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

sapphi-red commented Jul 3, 2022

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Uh oh!

sapphi-red Jul 3, 2022

Choose a reason for hiding this comment

Uh oh!

netlify bot commented Jul 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for vite-docs-main ready!

Uh oh!

patak-cat commented Jul 3, 2022

Uh oh!

brillout commented Jul 4, 2022

Uh oh!

patak-cat commented Jul 4, 2022

Uh oh!

brillout commented Jul 4, 2022

Uh oh!

aleclarson commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aleclarson Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

patak-cat commented Nov 16, 2022

Uh oh!

aleclarson commented Nov 16, 2022

Uh oh!

brillout commented Nov 16, 2022

Uh oh!

patak-cat commented Nov 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

netlify bot commented Jul 3, 2022 •

edited

Loading

aleclarson commented Nov 16, 2022 •

edited

Loading