search

mdx-js / mdx

Markdown for the component era
https://mdxjs.com
MIT License
17.77k stars 1.14k forks source link

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

Closed remcohaszing closed 1 year ago

remcohaszing commented 1 year ago

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[bot] commented 1 year ago

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 commented 1 year ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

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

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2281 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 23 23 Lines 2499 2499 Branches 4 4 ========================================= Hits 2499 2499 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wooorm commented 1 year ago

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

wooorm commented 1 year ago

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

remcohaszing commented 1 year ago

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 commented 1 year ago

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:

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Thank you!