-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
refactor: make HMR agnostic to environment #15179
Conversation
Run & review this pull request in StackBlitz Codeflow. |
/ecosystem-ci run |
This looks great to me, thanks for dividing the progress on mergeable chunks! About naming, given that we already have |
Yeah, I was off-put by uppercase letters there, but if we already do it like that, I think it's better to adopt the same style here for consistency. |
Ya, the |
📝 Ran ecosystem CI on
|
601df1f
…backs if needed This is needed when closing a server for example (need to wait!)
packages/vite/src/shared/hmr.ts
Outdated
public notifyListeners(event: string, data: any): unknown { | ||
const cbs = this.customListenersMap.get(event) | ||
if (cbs) { | ||
return cbs.map((cb) => cb(data)) |
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.
There was an open issue in vite-node when the user closed the server in this callback, but reload happened too fast and new server was not able to reuse the existing port (because it was still in use). I don't know if this is relevant for HMR in the browser so I didn't touch the behavior there, but I wanted to await here when reusing this method in vite-node
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.
Oh, I didn't know the value in disposeMap
and pruneMap
can return a promise. Doing return Promise.allSettled()
makes sense to me.
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.
void
allows returning a Promise
:P
function test(cb: () => void) {
cb()
}
test(() => {
return new Promise((r => r('1')))
})
Issue in vitest: vitest-dev/vitest#2334
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 am returning Promise<void>
from this method in the latest commit now
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.
Looks great! I like that the two concepts are now nicely grouped together.
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.
Big step forward!
Description
This PR makes core HMR implementation agnostic to the environment so it can be reused (and enhanced if needed) in other runtimes (Node/Deno/Bun) like in #12165. No logic is changed.
Unfortunately, I had to move it into another file which breaks git history for client file a bit :(
Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).