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(types): support custom VitePreloadErrorEvent #17615

Merged
merged 6 commits into from
Jul 30, 2024

Conversation

btea
Copy link
Collaborator

@btea btea commented Jul 5, 2024

Description

close #17508

Copy link

stackblitz bot commented Jul 5, 2024

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

packages/vite/client.d.ts Outdated Show resolved Hide resolved
@MathiasWP
Copy link

Would it also be relevant to cast the actual error being thrown in https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/importAnalysisBuild.ts#L148

Doing something like this in the importAnalysisBuild.ts file:

const e: VitePreloadErrorEvent = new Event('vite:preloadError', { cancelable: true })
e.payload = err
window.dispatchEvent(e)
if (!e.defaultPrevented) {
    throw err
}

I think that will also make it possible to remove the // @ts-expect-error comment

packages/vite/client.d.ts Outdated Show resolved Hide resolved
@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Jul 23, 2024
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@btea
Copy link
Collaborator Author

btea commented Jul 23, 2024

As @MathiasWP said, do you think we need to customize a VitePreloadErrorEvent type to remove ts comments? @bluwy

packages/vite/client.d.ts Outdated Show resolved Hide resolved
Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
bluwy
bluwy previously approved these changes Jul 23, 2024
@bluwy
Copy link
Member

bluwy commented Jul 23, 2024

As @MathiasWP said, do you think we need to customize a VitePreloadErrorEvent type to remove ts comments? @bluwy

If it's possible to cast then that would be great, but I don't know if there's complications doing that when the types come from client.d.ts. I don't mind if not either.

@btea
Copy link
Collaborator Author

btea commented Jul 23, 2024

The type is not complicated, it is simpler to just declare one.

@bluwy bluwy added this to the 5.4 milestone Jul 24, 2024
@bluwy bluwy merged commit 116e37a into vitejs:main Jul 30, 2024
11 checks passed
@btea btea deleted the feat/custom-VitePreloadErrorEvent branch July 30, 2024 08:37
@curtdept
Copy link

curtdept commented Aug 8, 2024

Feedback for whoever, in typescript this makes is more complicated to spyOn as there is now a colliding overload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Declare missing vite:preloadErrorevent on Window namespace
4 participants