react: fix to classify @types/react as a peer dependency

[remcohaszing]

Since react is a peerDependency, so should it’s matching type definitions be. However, in order not to bother people not using TypeScript, it’s made optional.

This solves dependency resolution for people using React lower than latest. A practical example:

Create an emtpy directory, and cd into it. Now run:

$ yarn add @mdx-js/react @types/react@17 react@17

And inspect the dependency tree:

$ yarn list                                      
yarn list v1.22.19
├─ @mdx-js/react@2.3.0
│  ├─ @types/mdx@^2.0.0
│  ├─ @types/react@>=16
│  └─ @types/react@18.0.31
│     ├─ @types/prop-types@*
│     ├─ @types/scheduler@*
│     └─ csstype@^3.0.2
├─ @types/mdx@2.0.4
├─ @types/prop-types@15.7.5
├─ @types/react@17.0.55
│  ├─ @types/prop-types@*
│  ├─ @types/scheduler@*
│  └─ csstype@^3.0.2
├─ @types/scheduler@0.16.3
├─ csstype@3.1.1
├─ js-tokens@4.0.0
├─ loose-envify@1.4.0
│  └─ js-tokens@^3.0.0 || ^4.0.0
├─ object-assign@4.1.1
└─ react@17.0.2
   ├─ loose-envify@^1.1.0
   └─ object-assign@^4.1.1

Alternatively:

$ find . -name package.json | sort
./node_modules/csstype/package.json
./node_modules/js-tokens/package.json
./node_modules/loose-envify/package.json
./node_modules/@mdx-js/react/node_modules/@types/react/package.json  # React 18 types
./node_modules/@mdx-js/react/package.json
./node_modules/object-assign/package.json
./node_modules/react/package.json                                    # React 17
./node_modules/@types/mdx/package.json
./node_modules/@types/prop-types/package.json
./node_modules/@types/react/package.json                             # React 17 types
./node_modules/@types/scheduler/package.json
./package.json

You can see there are two versions of @types/react. This may cause hard to debug conflicts.

This solves the problem people described in https://github.com/orgs/mdx-js/discussions/2238. This is not intentional. They are just lucky react doesn’t ship their own type definitions.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
mdx	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 19, 2023 1:05pm

[codecov-commenter]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (a34177c) 100.00% compared to head (1ddd6af) 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff            @@
##              main     #2281   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         2499      2499           
  Branches         4         4           
=========================================
  Hits          2499      2499           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[wooorm]

I think this will break several setups: https://github.com/orgs/mdx-js/discussions/2238#discussioncomment-5455250.

[wooorm]

If this is done, it should not be done to only react?

[remcohaszing]

This would only break for users who have relied on @types/react being a transitive dependency instead of an explicit one. TypeScript will show them an error they need to install @types/react manually, which they should have done in the first place.

This should be done for all frameworks that don’t ship their own types. By coincidence this is only react. preact and vue ship their own type definitions.

[wooorm]

You are completely right that that’s what should happen in npm. It is also theoretically better. But it depends on a a recent undocumented change in npm.

I am unsure about this change because we operate at a large scale with this very popular package.

There are several things that cause my hesitation:

• historically, we have experienced several times where peers didn’t work out, particularly around package managers that aren’t the latest npm: https://github.com/search?q=peerDependencies+OR+peerDependency+user%3Awooorm+user%3Amdx-js+user%3Amicromark+user%3Aremarkjs+user%3Arehypejs+user%3Aunifiedjs+user%3Asyntax-tree&type=issues
• historically, we have seen a lot of times where types changes broke things for folks (remember doing mayors any time types were added?)
• historically, we have had problems with things that should work assuming specs and ecosystem things, but didn’t (particularly for example dual publishing)

This makes me quite cautious about such changes at a large scale.
I am fine with testing the waters in smaller projects first.
I am not blocking this, but not approving it either.

[remcohaszing]

1. Dependencies indicate that a dependency should be resolvable for a package, but it’s ok if there are other versions somewhere else in node_modules.
2. Peer dependencies indicate there should be one deduplicated occurrence of a package.

Given a package b which has major versions 1 to 18.

1. If package a has a dependency on b>=16, a package manager is free to install either version 16, 17, or 18. This dependency may be located either in node_modules/b, or node_modules/a/node_modules/b.
2. If package a has a peer dependency on b>=16, a package manager is free to install either version 16, 17, or 18. This dependency must be located in node_modules/b. A package manager can either install this automatically, or delegate this responsibility to the user.

A libary for a framework should have a peer dependency on the framework. This the same for framework types. If a user uses react@16, they should also use @types/react@16.

Optional peer dependencies continues on the concept of peer dependencies. It means a b may be used if installed alongside a, but it’s not required for a to function. This applies to @types/react. @mdx-js/react can function fine without @types/react, but it’s required to make the TypeScript types work.

I’m pretty adamant @types/react belongs in peerDependencies alongside react. It’s already causing issues for Yarn users. I see this also matches what react-markdown does.

I also think @types/react should be optional, because otherwise it forces non-TypeScript users to install @types/react. IMO not ideal, but also it’s not that bad of an issue.

I can remove the peerDependenciesMeta if that’s what’s needed to convince you.

[wooorm]

I also think @types/react should be optional, because otherwise it forces non-TypeScript users to install @types/react. IMO not ideal, but also it’s not that bad of an issue.

I don’t think whether someone uses TS or not can be inferred from optional? I feel like we discussed this elsewhere?
Given the package managers are different, I’d assume some will install it for users that don’t use TS, and that some won’t for users that do?

[remcohaszing]

I don’t think whether someone uses TS or not can be inferred from optional?

No, it can’t. Unfortunately npm and other package managers don’t have the concept of optional dependencies based on context. An optional peer dependency basically means the package manager will show a warning or error if a non-matching version is installed alongside the package. It’s kind of similar to the engines field.

[wooorm]

Thank you!