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(create-app): improve client types #3214

Merged
merged 4 commits into from
May 22, 2021
Merged

Conversation

fi3ework
Copy link
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
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
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
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
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
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
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
Member Author

fi3ework commented May 22, 2021

Also updated the doc of Client Types 🤠

Shinigami92
Shinigami92 previously approved these changes May 22, 2021
docs/guide/features.md Outdated Show resolved Hide resolved
@patak-dev patak-dev changed the title chore(create-app): remove tsconfig.types feat(create-app): improve client types May 22, 2021
@patak-dev patak-dev 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