-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
feat: support import.meta.hot.invalidate #10244
Conversation
Self-accepting modules can call this to continue propagating the HMR update to importers. Conditional `hot.accept` calls do not work unless `hot.invalidate` is called when the condition fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I have some questions to understand this more deeper.
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: 翠 / green <green@sapphi.red>
Not sure what the failing tests are about, maybe they're flaky? |
Co-authored-by: Alec Larson <1925840+aleclarson@users.noreply.github.com>
Sorry, I just remembered we have types for HMR events here. Would you add vite/packages/vite/src/types/customEvent.d.ts Lines 1 to 13 in 81d4d04
|
I started to do that before, but then I need to add a handler to Should I add a type, but then not add it to the union of |
packages/vite/src/client/client.ts
Outdated
// TODO should tell the server to re-perform hmr propagation | ||
// from this module as root | ||
location.reload() | ||
notifyListeners('vite:invalidate', ownerPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sapphi is talking about this client-side event, I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this client event is even worth adding? What's the use case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume the use case for all of these is to help debug HMR problems and also to confirm they're working as expected in tests.
If I add the type to the HMRPayload union, then I'd need to handle it where I pointed out because of the exhaustive switch statement. I could create a separate type that's not included in that union, but it would be different from the others. But, I guess it is different, so maybe that's fine.
I'm also happy to remove the notifyListeners if that's preferred.
vite/packages/vite/src/node/server/ws.ts Line 34 in 81d4d04
vite/packages/vite/src/node/server/ws.ts Line 45 in 81d4d04
vite/packages/vite/src/types/hot.d.ts Lines 28 to 32 in 81d4d04
vite/packages/vite/src/client/client.ts Line 253 in 81d4d04
It seems |
2b9477b
to
f813817
Compare
f813817
to
d147d6b
Compare
@sapphi-red I'm not sure if I did this correctly, since it seemed like I needed to re-export the type from a lot of files, but at least the build is passing for me locally. The macos failure here looks like flake, I think. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 💚 LGTM
Description
Self-accepting modules can call this to continue propagating the HMR update to importers. Conditional
hot.accept
calls do not work unlesshot.invalidate
is called when the condition fails.This was pulled out from #10239, with some docs and tests added. But the real credit here goes to @aleclarson.
Additional context
This is needed for propagating HMR updates from files that should not be treated as react fast-refresh boundaries according to the runtime check.
I think this does not change behavior enough to warrant a breaking change label, but it does change the behavior from simply refreshing the page to propagating HMR upwards. But it's possible that some users were using
invalidate()
because they wanted a full page refresh.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).