-
-
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: add client events to import.meta.hot.on #3638
Conversation
idea: IMO we could try to find a better more expressive name 🤔 |
Pretty much! I like that approach. If we were to do this, then I guess we'd also want to enable listening to other events such as successful connection to the Websocket, HMR reloads, and so on. This would actually be useful for me as well, as I'm currently having to monkey-patch I'll update this PR later today! |
@Shinigami92 I did something a bit simpler and added a general |
Sorry to inform you, but I will not have much time the next few days 🙁 IMO: yeah could be okay this way how it's currently implemented, but using EventEmitter could be nicer and stronger in the future 🤔 🤷 I don't know 🤔 |
I've integrated @Shinigami92's suggestion from fwouts#1 and added some types + basic tests. One thing I realised is that, because the import needs to be |
I provided just some minor improvements Initially I thought we need to pass I also updated the tests to confirm that the correct detail was passed But we may need to define tests for error and connected 🤔 @fwouts It's now up to you to define the first set of which events we currently need 🙂 Beside that, I think you need to rebase the branch after you merged my second suggestions 😅 |
And sorry when I'm not responsive today anymore, real life is catching up 😄 |
Thank you! And don't worry you've been extremely responsive, much appreciated. FYI now that #3677 is approved, this PR isn't blocking me anymore, so no rush at all 🙂 I'll look at the PR within the next day. |
Hey @fwouts, in the team we discussed that there is already a We should rebuild this PR to use it and make it possible to use e.g. import.meta.hot.on('vite:connected', (...) => { ... }) |
Sounds good! Makes me feel better about not having time for this PR last week :) |
feat: minor improvements
This event cannot be relied upon because there is a race condition between the WebSocket connection, established as soon as client.js is run, and whatever subsequent code actually invokes import.meta.hot.on()
@Shinigami92 This is now updated to use |
Do you want also to extend the docs? So maybe we can list which (known) events can be listened |
Good suggestion. I've added documentation, I figure it's not obvious unless mentioned :) |
type CustomEventName<T extends string> = (T extends `vite:${T}` ? never : T) & | ||
(`vite:${T}` extends T ? never : T) |
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.
Mh 🤔 if custom events from plugins can also be send, should we enforce the type to start with vite:
always?
@patak-js What do you think?
Otherwise I now approve this PR from my side
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 don't know if I understand the question, we should enforce vite:
for our events, and custom events are free to use their own naming convention.
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 don't know if I understand the question, we should enforce vite:
for our events, and custom events are free to use their own naming convention.
Looks good! My only doubt is about naming and timing of notifications. Right now the notification happens before the vite takes care of the event. Should |
Great suggestion @patak-js. I've updated the event names and doc. I tried implementing |
I will add this to the list and ask for Evan for the final review. |
Co-authored-by: Shinigami92 <[email protected]> Co-authored-by: Christopher Quadflieg <[email protected]>
Description
This adds an exported function
setCustomErrorHandler
to the@vite/client
package.Example usage:
This is useful when neither Vite's HMR overlay or the fallback
console.error
implementation are sufficient. For example, in the context of reactpreview.com, Vite is running in an iframe which needs to send the full error to the parent frame, where it's rendered appropriately (with custom styling).Additional context
N/A
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).