-
-
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
refactor(types): simplify type exports #10243
Conversation
Awesome! Downsides of doing this that I could think are:
(sorry that I forgot about 2 and 3 when discussing before) That said, I think this is nicer than mine. I suppose 1 is acceptable. For 2 and 3, maybe we could still have something like |
Co-authored-by: 翠 / green <green@sapphi.red>
Thanks for the review! Yeah no1 had always felt like an odd feature from api-extractor, but I think it'd be fine too. Re no2 and 3, I think requiring I don't understand how exporting |
This is a reproduction of no2.
If |
Thanks for the explanation! I'm starting to understand the issue now. So if someone wants to import a type that doesn't also bring in I guess my thoughts are:
But if we want to support this, I'd propose exporting via Perhaps we can tackle this with a separate PR, but I'm also happy to implement something at the meantime too. |
If we removed
Yeah, I agree it's difficult to tell people about this.
I don't have a good idea so I'm fine for now with this implementation. 🙂 |
Merging with approval from @patak-dev |
Description
This follows up on #9966. (Sorry for the wall of text)
Reason for change
vite/client/types
is confusing withvite
andvite/client
.For example, you can import
HMRPayload
in bothvite/client/types
andvite
. (I think the latter should only)vite/import-meta
feels very specific.I'm discussing with @Princesseuh which maybe we can try to use
vite/client
if possible. I think we can removevite/import-meta
.import.meta
givesImportMeta_2
due to bundling.I feel like this may bring a confusing experience for some people, and we should try sticking back to
ImportMeta
CustomEventMap
twice can be avoidable.We need to deduplicate
CustomEventMap
so users only have to define once.PR changes
src/types/
back totypes/
. Movesrc/dep-types/
tosrc/types/
I'll explain why I moved back to
types/
below. But renamingdep-types
totypes
is mainly a small annoyance i find of the directory showingtypes/
are deduplicated (aka shared between client and node types)This is done by making a post patch to the types in
postPatchTypes.ts
. Rewriting imports oftypes/*
->../../types/*
.ImportMeta_2
, and removevite/client/types
andvite/import-meta
This one is tricky and is the main reason I move
src/types/
back totypes/
. We need to define theImportMeta
manually to avoid the issue, but I find bundling the client types getting in the way, so I went back to the structure before.I also don't see much usecase for
vite/client/types
(initially I wanted to rename this asvite/shared
). But all types can be imported fromvite
instead, so I remove it to prevent duplication.To be deprecated
types/<dep>
(types/importGlob
,types/customEvent
etc are fine but should be avoided unless documented)Note: I did not deprecated
types/importGlob
which initially is superseded byvite/client/types
due to the above reasons.What is the purpose of this pull request?