Refine how TypeScript types are handled

[Timer]

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]

@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]

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]

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]

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]

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]

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]

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]

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]

Wow, that's actually an excellent idea! 🤯

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

[ianschmitz]

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]

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

1. Should they be dependencies of react-scripts?
2. Dependencies of @types/react-script-env?
3. Peer dependencies of @types/react-script-env?
4. A combination of these?

[Timer]

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]

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

1. Should they be dependencies of react-scripts?
2. Dependencies of @types/react-script-env?
3. Peer dependencies of @types/react-script-env?
4. 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]

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

[ianschmitz]

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]

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

[Timer]

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.