Skip to content

refactor(types): simplify type exports#10243

Merged
bluwy merged 11 commits intomainfrom
types-attempt
Sep 30, 2022
Merged

refactor(types): simplify type exports#10243
bluwy merged 11 commits intomainfrom
types-attempt

Conversation

@bluwy
Copy link
Member

@bluwy bluwy commented Sep 25, 2022

Description

This follows up on #9966. (Sorry for the wall of text)

Reason for change

  1. The new exported vite/client/types is confusing with vite and vite/client.

For example, you can import HMRPayload in both vite/client/types and vite. (I think the latter should only)

  1. 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 remove vite/import-meta.

  1. Hovering over import.meta gives ImportMeta_2 due to bundling.

I feel like this may bring a confusing experience for some people, and we should try sticking back to ImportMeta

  1. Defining the CustomEventMap twice can be avoidable.

We need to deduplicate CustomEventMap so users only have to define once.

PR changes

  1. Move src/types/ back to types/. Move src/dep-types/ to src/types/

I'll explain why I moved back to types/ below. But renaming dep-types to types is mainly a small annoyance i find of the directory showing

src
- client
- dep-types <-- between client/node
- node
  1. All types in types/ 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 of types/* -> ../../types/*.

  1. Fix ImportMeta_2, and remove vite/client/types and vite/import-meta

This one is tricky and is the main reason I move src/types/ back to types/. We need to define the ImportMeta 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 as vite/shared). But all types can be imported from vite 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 by vite/client/types due to the above reasons.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@bluwy bluwy requested a review from sapphi-red September 25, 2022 12:27
@sapphi-red
Copy link
Member

sapphi-red commented Sep 25, 2022

Awesome!

Downsides of doing this that I could think are:

  1. we won't be able to use api-extractor features in packages/vite/types (e.g. removing @internal properties).
  2. typescript will show error when
    • this condition is met (related: vite config files can't be compiled without dom types #9813)
      • import { HMRPayload } from 'vite' (and other types) is imported
      • @types/node is not referenced
      • skipLibCheck: false
    • this could be workarounded by either
      • using import { HMRPayload } from 'vite/types/hmrPayload' that is not documented
      • add @types/node (node globals is not accessible from client code though)
      • skipLibCheck: true (not ideal)
  3. the opposite of above (vite config files can't be compiled without dom types #9813)

(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 vite/client/types that will be included in exports?

sapphi-red
sapphi-red reviewed Sep 25, 2022
View reviewed changes
@bluwy @sapphi-red
docs: remove stale comment
5366624 
Co-authored-by: 翠 / green <green@sapphi.red>
@bluwy
Copy link
Member Author

bluwy commented Sep 25, 2022

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 @types/node is fair especially if they're planning to compile with tsc. I'd usually lean towards skipLibCheck: true too (personal preference 😬 )

I don't understand how exporting vite/client/types would solve #9813 though. IIRC it will still cause the issue. Can you elaborate on that?

@bluwy bluwy added the p2-nice-to-have Not breaking anything but nice to have (priority) label Sep 25, 2022
@sapphi-red
Copy link
Member

sapphi-red commented Sep 26, 2022

Re no2 and 3, I think requiring @types/node is fair especially if they're planning to compile with tsc. I'd usually lean towards skipLibCheck: true too (personal preference 😬 )

This is a reproduction of no2.
Since node related types are used in vite/node/index.d.ts, @types/node needs to be installed. Also if it's installed, it's always referenced (because vite/node/index.d.ts includes /// <reference types="node" />, it seems this can't be avoided by using compilerOptions.types). So when vite is imported, process will be shown as if it is declared.

I don't understand how exporting vite/client/types would solve #9813 though. IIRC it will still cause the issue. Can you elaborate on that?

If vite/client/types doesn't include node related types and doesn't depend on vite/node/index.d.ts, @types/node won't be referenced and won't be needed. So process won't be shown as if it is declared.

@bluwy
Copy link
Member Author

bluwy commented Sep 26, 2022

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 node types, they can import from vite/client/types instead. Was confused as #9813 mentions dom types.

I guess my thoughts are:

  1. Not everyone knows when to import from vite or vite/client/types.
  2. Maybe they don't mind node types in client code.

But if we want to support this, I'd propose exporting via vite/shared 🤔 and we can bundle up the types within types/ (besides importMeta). I think it might be useful to support this, but I'm not sure the usecases out there that requires this.

Perhaps we can tackle this with a separate PR, but I'm also happy to implement something at the meantime too.

@sapphi-red
Copy link
Member

sapphi-red commented Sep 28, 2022

Was confused as #9813 mentions dom types.

If we removed importGlob related types from vite/node/index.d.ts, we could solve #9813 (though we still have the error coming from esbuild's type def).
So we could solve #9813 (Importing vite/node/index.d.ts without adding DOM to tsconfig.compilerOptions.lib) and the opposite (what I said in the previous comment; importing client related types without node types).

  1. Not everyone knows when to import from vite or vite/client/types.

Yeah, I agree it's difficult to tell people about this.

I'd propose exporting via vite/shared 🤔

types/importMeta needs to be externalized from vite/shared to avoid referring dom types.

Perhaps we can tackle this with a separate PR, but I'm also happy to implement something at the meantime too.

I don't have a good idea so I'm fine for now with this implementation. 🙂

sapphi-red
sapphi-red previously approved these changes Sep 28, 2022
View reviewed changes
@bluwy
Copy link
Member Author

bluwy commented Sep 30, 2022

Merging with approval from @patak-dev

@bluwy bluwy enabled auto-merge (squash) September 30, 2022 09:16
@bluwy bluwy merged commit 291174d into main Sep 30, 2022
@bluwy bluwy deleted the types-attempt branch September 30, 2022 09:29
@sapphi-red sapphi-red mentioned this pull request Nov 13, 2022
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@patak-cat patak-cat patak-cat approved these changes

@sapphi-red sapphi-red sapphi-red left review comments

Assignees

No one assigned

Labels

p2-nice-to-have Not breaking anything but nice to have (priority)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bluwy @sapphi-red @patak-cat