Skip to content

feat(create-app): improve client types#3214

Merged
patak-cat merged 4 commits intovitejs:mainfrom
fi3ework:fix-types
May 22, 2021
Merged

feat(create-app): improve client types#3214
patak-cat merged 4 commits intovitejs:mainfrom
fi3ework:fix-types

Conversation

@fi3ework
Copy link
Copy Markdown
Member

@fi3ework fi3ework commented Apr 29, 2021

Description

Definition of tsconfig.types on the TS documentation writes

If types is specified, only packages listed will be included in the global scope. For instance:

So if use Vite to create a *-ts project and TS will not recognize global definition such as Jest unless add Jest to tsconfig.types.

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@fi3ework fi3ework changed the title fix(create-app): remove tsconfig.types chore(create-app): remove tsconfig.types Apr 29, 2021
@Shinigami92 Shinigami92 added the p2-nice-to-have Not breaking anything but nice to have (priority) label Apr 30, 2021
@fi3ework
Copy link
Copy Markdown
Member Author

fi3ework commented May 20, 2021

Anyone would like to review this PR? cc @Shinigami92 😉

nihalgonsalves
nihalgonsalves previously approved these changes May 20, 2021
@Shinigami92
Copy link
Copy Markdown
Member

I'm not so sure about this PR 🤔
But I'm also not biased about it...

On the one side, if someone really needs it, they could easily just add types to the types field or change it just on their own to this solution.
On the other side, I think I saw some other templates from other tools that also just generated like this way with types.

e.g. for tests, I personally have an extra tsconfig.json file for my tests and define cypress and jest there so they don't interfere each other.

Are there more benefits of doing this? Do we have some real world examples?

@nihalgonsalves
Copy link
Copy Markdown
Member

nihalgonsalves commented May 20, 2021

@Shinigami92 I'd say setting types is a less sensible default than providing a globals file. As a user you'd expect Jest and other global types to just work, but if we pre-set types then users will have to find out why their project is broken (when they add a new package and the types don't work).

Next.js also uses this approach, creating a next-env.d.ts file with the client types referenced.

Cypress is an exception since the Cypress & Jest globals interfere with each other, and like you said, that is normally a separate tsconfig in any case.

In general you shouldn't have to make a config change for each package you add [that affects the global types].

@fi3ework
Copy link
Copy Markdown
Member Author

Yes, remove compilerOptions.types and adding a global declaration file won't block other new added global types. create-react-app also init a react-app-env.d.ts for TypeScript template. And maybe we can tweak and unify the d.ts naming.

@Shinigami92
Copy link
Copy Markdown
Member

Okay, so what about naming the file vite-env.d.ts

Also we could add a documentation section for explaining this file, like in the Next.js docs.
Please also check if there isn't already "old" documentation that needs to be updated.

Shinigami92
Shinigami92 previously approved these changes May 21, 2021
@Shinigami92
Copy link
Copy Markdown
Member

Have tested it and yes it's working, please rename the file and then we can merge it

@fi3ework fi3ework dismissed stale reviews from Shinigami92 and nihalgonsalves via fe4ea01 May 22, 2021 10:08
@fi3ework
Copy link
Copy Markdown
Member Author

fi3ework commented May 22, 2021

Also updated the doc of Client Types 🤠

Shinigami92
Shinigami92 previously approved these changes May 22, 2021
@patak-cat patak-cat changed the title chore(create-app): remove tsconfig.types feat(create-app): improve client types May 22, 2021
@patak-cat patak-cat merged commit ba8b7af into vitejs:main May 22, 2021
@fi3ework fi3ework deleted the fix-types branch May 22, 2021 14:17
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.

4 participants