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

feat: support import.meta.hot.invalidate #10244

Merged
merged 14 commits into from
Sep 28, 2022
Merged

Conversation

IanVS
Copy link
Contributor

@IanVS IanVS commented Sep 25, 2022

Description

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.

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?

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

aleclarson
aleclarson previously approved these changes Sep 25, 2022
Copy link
Member

@sapphi-red sapphi-red left a 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.

packages/vite/src/node/server/index.ts Outdated Show resolved Hide resolved
packages/vite/src/node/server/index.ts Outdated Show resolved Hide resolved
playground/hmr/invalidation/child.js Outdated Show resolved Hide resolved
@sapphi-red sapphi-red added feat: hmr p3-significant High priority enhancement (priority) labels Sep 26, 2022
Co-authored-by: 翠 / green <green@sapphi.red>
IanVS and others added 2 commits September 26, 2022 09:45
@IanVS
Copy link
Contributor Author

IanVS commented Sep 26, 2022

Not sure what the failing tests are about, maybe they're flaky?

aleclarson
aleclarson previously approved these changes Sep 26, 2022
docs/guide/api-hmr.md Outdated Show resolved Hide resolved
Co-authored-by: Alec Larson <1925840+aleclarson@users.noreply.github.com>
aleclarson
aleclarson previously approved these changes Sep 26, 2022
@sapphi-red
Copy link
Member

Sorry, I just remembered we have types for HMR events here. Would you add vite:invalidate?

import type {
ErrorPayload,
FullReloadPayload,
PrunePayload,
UpdatePayload
} from './hmrPayload'
export interface CustomEventMap {
'vite:beforeUpdate': UpdatePayload
'vite:beforePrune': PrunePayload
'vite:beforeFullReload': FullReloadPayload
'vite:error': ErrorPayload
}

@IanVS
Copy link
Contributor Author

IanVS commented Sep 27, 2022

Sorry, I just remembered we have types for HMR events here. Would you add vite:invalidate?

I started to do that before, but then I need to add a handler to handleMessage, but those seem like messages from the server to the client, whereas the invalidate message is going the other direction, right?

Should I add a type, but then not add it to the union of HMRPayload?

// TODO should tell the server to re-perform hmr propagation
// from this module as root
location.reload()
notifyListeners('vite:invalidate', ownerPath)
Copy link
Member

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

Copy link
Member

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?

Copy link
Contributor Author

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.

@sapphi-red
Copy link
Member

send<T extends string>(event: T, payload?: InferCustomEventPayload<T>): void

listener: WebSocketCustomListener<InferCustomEventPayload<T>>

on<T extends string>(
event: T,
cb: (payload: InferCustomEventPayload<T>) => void
): void
send<T extends string>(event: T, data?: InferCustomEventPayload<T>): void

data: InferCustomEventPayload<T>

It seems InferCustomEventPayload is used for both client -> server and server -> client.
So I think addding to CustomEventMap and not adding to HMRPayload makes sense.

@IanVS
Copy link
Contributor Author

IanVS commented Sep 28, 2022

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

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thanks 💚 LGTM

@aleclarson aleclarson merged commit fb8ab16 into vitejs:main Sep 28, 2022
@IanVS IanVS deleted the hot-invalidate branch September 28, 2022 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants