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

refactor: make HMR agnostic to environment #15179

Merged
merged 5 commits into from Dec 1, 2023

Conversation

sheremet-va
Copy link
Member

@sheremet-va sheremet-va commented Nov 29, 2023

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?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

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

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • 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).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Nov 29, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev
Copy link
Member

/ecosystem-ci run

@patak-dev
Copy link
Member

This looks great to me, thanks for dividing the progress on mergeable chunks! About naming, given that we already have HMRPayload, should we call the client HMRClient? (and the instance hmrClient, hmrContext to be more explicit)

@sheremet-va
Copy link
Member Author

This looks great to me, thanks for dividing the progress on mergeable chunks! About naming, given that we already have HMRPayload, should we call the client HMRClient? (and the instance hmrClient, hmrContext to be more explicit)

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.

@patak-dev
Copy link
Member

This looks great to me, thanks for dividing the progress on mergeable chunks! About naming, given that we already have HMRPayload, should we call the client HMRClient? (and the instance hmrClient, hmrContext to be more explicit)

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 HMRC is quite a mouthful, but I think "hmr client" reads a lot better than "client hmr" so it kind of compensates.

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 274c709: Open

suite result latest scheduled
analogjs success success
astro success success
histoire failure failure
ladle success success
laravel failure failure
marko success success
nuxt failure failure
nx success success
previewjs success success
qwik success success
rakkas success success
sveltekit failure failure
unocss success success
vike success success
vite-plugin-pwa success success
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc success success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress success success
vitest success success

@sheremet-va sheremet-va changed the title refactor: move hmr to "shared" folder refactor: make HMR agnostic to environment Nov 29, 2023
patak-dev
patak-dev previously approved these changes Nov 29, 2023
sapphi-red
sapphi-red previously approved these changes Nov 30, 2023
packages/vite/src/client/client.ts Outdated Show resolved Hide resolved
…backs if needed

This is needed when closing a server for example (need to wait!)
public notifyListeners(event: string, data: any): unknown {
const cbs = this.customListenersMap.get(event)
if (cbs) {
return cbs.map((cb) => cb(data))
Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member Author

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

Copy link
Member

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

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Big step forward!

@patak-dev patak-dev merged commit 0571b7c into vitejs:main Dec 1, 2023
7 of 11 checks passed
@patak-dev patak-dev added the p3-significant High priority enhancement (priority) label Dec 1, 2023
@sheremet-va sheremet-va deleted the refactor/hmr-env-agnostic branch December 1, 2023 10:26
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.

None yet

4 participants