Skip to content

feat: add EventTarget to @vite/client#1

Merged
fwouts merged 1 commit intofwouts:custom-error-handlerfrom
vitejs:client-event-target
Jun 4, 2021
Merged

feat: add EventTarget to @vite/client#1
fwouts merged 1 commit intofwouts:custom-error-handlerfrom
vitejs:client-event-target

Conversation

@Shinigami92
Copy link
Copy Markdown

@Shinigami92 Shinigami92 commented Jun 4, 2021

This is a suggested alternative to vitejs#3638

@fwouts && @patak-js please have a look

Also @fwouts please check this out locally and test it with

import { viteEventTarget } from "@vite/client";

viteEventTarget.addEventListener('vite:error', (event) => {
  const err = (event as CustomEvent).detail.err;
  showNotification(err.message + '\n' + err.stack);
});

I just written the code without testing anything!!!
Please also think about adding more events.
If this is a nice solution and working, we need to improve the types / override the typedefs for the addEventListener call so it's not just string, but the different events + inline docs.
Documentation needs also be updated.
It's also maybe possible to write tests 🤔

@fwouts
Copy link
Copy Markdown
Owner

fwouts commented Jun 4, 2021

Nice one! I'll try to test this out later this weekend.

In terms of tests, I can think of a couple of approaches:

  • unit testing either by mocking out the Websocket connection or actually running a Websocket server within the tests
  • integration testing by using Puppeteer/Playwright to run a browser, asserting the expected behaviour

The latter could fit nicely into the existing tests in https://github.com/vitejs/vite/blob/eb66b4350c635fb4f2ef2e8a9eb50958cde73743/packages/playground/hmr/__tests__/hmr.spec.ts

In terms of unit testing, I personally feel like we need to refactor and split up client.ts to make it more modular and testable first (and that should be its own PR). Otherwise, these unit tests will likely look too hacky.

Would integration tests be sufficient in this instance?

@fwouts fwouts merged commit 795ab4b into fwouts:custom-error-handler Jun 4, 2021
@fwouts
Copy link
Copy Markdown
Owner

fwouts commented Jun 4, 2021

Yup, I can confirm this works :) I'll try and improve it further as you suggested.

@fwouts fwouts mentioned this pull request Jun 4, 2021
Merged
9 tasks
@bluwy bluwy deleted the client-event-target branch July 24, 2024 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Shinigami92 @fwouts