Refine how TypeScript types are handled by Timer · Pull Request #5593 · facebook/create-react-app

Timer · 2018-10-26T20:29:43Z

Right now, we copy our react-app.d.ts file into the user's source folder to register types for our webpack loaders, e.g.:

declare module '*.svg' {
  import * as React from 'react';

  export const ReactComponent: React.SFC<React.SVGProps<SVGSVGElement>>;

  const src: string;
  export default src;
}

declare module '*.module.css' {
  const classes: { [key: string]: string };
  export default classes;
}

I feel like this isn't very optimal, and I'd rather this be "encapsulated" somehow.

On first thought, it looks like we could publish @types/react-scripts and have the types automatically registered -- however, I'm not sure we want that maintenance overhead when the types field exists in package.json.

This pull request is exploring how we could rely on the types key so our types are always specific to the react-scripts version without relying on package manager hoisting metrics/external packages.

It looks like we can trigger TypeScript to import these types by specifiying "types": ["react-scripts"] in tsconfig.json, but this turns off the default behavior of absorbing the @types/ folder.

Also, using include in tsconfig.json seems suboptimal because we can't specify node_modules/react-scripts/config/react-app.d.ts since it might be hoisted.

I suppose the crux of the problem is there's never a time where someone would write import 'react-scripts';.

DanielRosenwasser · 2018-10-26T23:53:53Z

@weswigham I think you brought this up when we discussed the changes PR earlier. I feel like a global .d.ts is pretty reasonable given that it's app-specific behavior from a loader.

Timer · 2018-10-27T00:40:30Z

By global .d.ts are you referring to us copying the file? Or a new behavior to allow something like this for toolkits/toolboxes to leverage?

p.s. thanks for the quick response!

weswigham · 2018-10-27T00:48:18Z

Yeah, I mentioned you could publish the file to DefinitelyTyped and then list it as a dependency, just like @types/node or @types/webpack-env. Type definitions on DT for specificing certain loader/runtime environments are fine. In theory you shouldn't need to specify the explicit types dependency then, since in the absence of an explicitly specified types option, all @types/ definitions locatable within a project are loaded. So yeah, it's a bit more to maintain, but way more automatic, in terms of configuration.

Timer · 2018-10-27T00:49:35Z

I'm worried about versioning and keeping them in sync, would we be able to conceal the dependency from the user by making react-scripts depend on a specific version of the types? I guess we'd be relying on hoisting metrics a bit then, and users could unexpectedly install an improper version.

weswigham · 2018-10-27T00:53:14Z

You could list it as a... Peer dependency, I think? So npm will warn if the versions in their project and what the scripts think is right don't match sufficiently.

Timer · 2018-10-27T00:54:35Z

Since TypeScript is optional we'd need to wait for the optional peer dependencies specification to be finalized between Yarn and npm 😅. Maybe copying the file is the best behavior for now...

Timer · 2018-10-27T01:07:08Z

Note: I don't want to go against TypeScript community convention, but I'm not sure if @types/react-scripts is the best route. Users who eject will need to drop this dependency and get a copy of the types file so they can add their modifications.

Because of the above constraint, I feel like it might be best to be consistent and copy the file so there's a better eject story.

I'm more than open to an alternate way to do this than copying a type declaration file into the user's directory. For the meantime, I've altered this PR to restore the copy behavior with additions of type references.
Do you think this is permissible or the best way to do this given the constraints?

weswigham · 2018-10-27T01:17:06Z

You could try making @types/react-scripts-env just be ///<reference types="react-scripts" /> and have people install @types/react-scripts-env as a kind of forwarding service to autoload the environment types.

Timer · 2018-10-27T01:25:42Z

Wow, that's actually an excellent idea! 🤯

Thanks for the suggestion, I think that'll probably work best.

ianschmitz · 2018-10-27T04:25:10Z

I'm worried about versioning and keeping them in sync, would we be able to conceal the dependency from the user by making react-scripts depend on a specific version of the types? I guess we'd be relying on hoisting metrics a bit then, and users could unexpectedly install an improper version.

Yes this should work @Timer. As we were chatting earlier, this would be picked up automatically by TypeScript (as well as i was mentioning VSCode should pick this up for javascript projects as well in my experience). Are you mainly concerned with @types/react-scripts not ending up in the root of node_modules or something similar?

Timer · 2018-10-27T16:12:09Z

What's best practice to specify our type dependencies (@types/react, @types/react-dom, @types/jest, and @types/node)?

Should they be dependencies of react-scripts?
Dependencies of @types/react-script-env?
Peer dependencies of @types/react-script-env?
A combination of these?

Timer · 2018-10-27T16:13:01Z

Also, is this best practice since we need the types to be installed?
https://github.com/facebook/create-react-app/pull/5593/files#diff-c2f8d8ef1abb763b0555188117513867R5

Are you mainly concerned with @types/react-scripts not ending up in the root of node_modules or something similar?
@ianschmitz

Yes, that's my concern.

e.g. node_modules/react-scripts/node_modules/@types/react-scripts

ianschmitz · 2018-10-27T21:24:37Z

What's best practice to specify our type dependencies (@types/react, @types/react-dom, @types/jest, and @types/node)?

Should they be dependencies of react-scripts?

Dependencies of @types/react-script-env?

Peer dependencies of @types/react-script-env?

A combination of these?

IMO:

@types/react and @types/react-dom should be controlled by the user as they correlate with the react and react-dom dependencies which are user controlled
@types/jest i feel should be controlled by CRA. This was an easy source of confusion in CRA-TS as jest was an internally controlled dependency (which was 20.x or 21.x at the time, i forget) but it was easy for a user to accidentally install @types/[email protected] which would give typings to the wrong APIs
@types/node is an interesting one.. not sure what do to do there. as you mentioned it seems specific to the node version that the user has installed

Another thought if you didn't want to manage an @types package, is you could add an entry to the include of tsconfig.json to point at node_modules/react-scripts/declarations/* or a similar path. Typescript will pick up any *.d.ts files in that path and include the typings in the project.

Timer · 2018-10-28T02:19:42Z

node_modules/react-scripts/declarations/* this has the same problem with the root location, no?

ianschmitz · 2018-10-28T02:46:33Z

node_modules/react-scripts/declarations/* this has the same problem with the root location, no?

I think since react-scripts is a dependency of the user's project, we can guarantee that there will be a node_modules/react-scripts folder?

Timer · 2018-10-28T02:50:55Z

For a while until Yarn PnP is more widespread. Can you DM me on Twitter @timer150?

Timer · 2018-10-28T05:10:19Z

We decided to not publish a types package, but instead use a derivative of the suggested env package.
We'll now create a file forwarding to the react-scripts package. This solves original concerns because it's a one time operation and doesn't cause git diff noise between versions or lints.

Please review this PR and let me know if this seems reasonable! I'm going to merge for now because it's an improvement.

* Specify types in package * Do not remove types file on eject * Stop copying types into generated project * Reference react and react-dom * Reference node types * Install node types as well * Restore copying * Add Node to the list of installed types * Reference Jest types * Remove jest types from install * Remove jest from CRA install * Remove Jest reference and let user do this themselves * Stop copying types file * Add types key to package.json * Add appTypeDeclarations and create when missing * Rename declarations file * Add Jest back to install instructions * Minimize diff

Timer added 2 commits October 26, 2018 13:20

Specify types in package

1ad63b4

Do not remove types file on eject

e7deb7d

Timer added the tag: enhancement label Oct 26, 2018

Timer added this to the 2.1 milestone Oct 26, 2018

facebook-github-bot added the CLA Signed label Oct 26, 2018

Timer added 2 commits October 26, 2018 13:30

Stop copying types into generated project

0ebcbbd

Reference react and react-dom

59a595e

Reference node types

290e128

Install node types as well

6fc9745

Timer added 2 commits October 26, 2018 18:07

Restore copying

b19c140

Add Node to the list of installed types

2a2cb5e

Timer changed the title ~~Add types entry to react-scripts~~ Refine how TypeScript types are handled Oct 28, 2018

Timer added 4 commits October 27, 2018 22:33

Reference Jest types

9301692

Remove jest types from install

322280e

Remove jest from CRA install

ca5ba7e

Remove Jest reference and let user do this themselves

39d227a

Stop copying types file

468cf09

Timer added 5 commits October 27, 2018 22:56

Add types key to package.json

0dcb0a4

Add appTypeDeclarations and create when missing

cc483f4

Rename declarations file

2f8ad10

Add Jest back to install instructions

5ee0a11

Minimize diff

174c386

Timer merged commit eca0ec0 into facebook:master Oct 28, 2018

Timer deleted the refactor/do-not-expose-types branch October 28, 2018 05:11

Timer added the issue: typescript label Oct 28, 2018

lock Bot locked and limited conversation to collaborators Jan 18, 2019

Uh oh!

Conversation

Timer commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DanielRosenwasser commented Oct 26, 2018

Uh oh!

Timer commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weswigham commented Oct 27, 2018

Uh oh!

Timer commented Oct 27, 2018

Uh oh!

weswigham commented Oct 27, 2018

Uh oh!

Timer commented Oct 27, 2018

Uh oh!

Timer commented Oct 27, 2018

Uh oh!

weswigham commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Timer commented Oct 27, 2018

Uh oh!

ianschmitz commented Oct 27, 2018

Uh oh!

Timer commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Timer commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ianschmitz commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Timer commented Oct 28, 2018

Uh oh!

ianschmitz commented Oct 28, 2018

Uh oh!

Timer commented Oct 28, 2018

Uh oh!

Timer commented Oct 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Timer commented Oct 26, 2018 •

edited

Loading

Timer commented Oct 27, 2018 •

edited

Loading

weswigham commented Oct 27, 2018 •

edited

Loading

Timer commented Oct 27, 2018 •

edited

Loading

Timer commented Oct 27, 2018 •

edited

Loading

ianschmitz commented Oct 27, 2018 •

edited

Loading

Timer commented Oct 28, 2018 •

edited

Loading