Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

mobx-react-lite does not specify react-dom nor react-native as it's peer dependencies #3885

Closed
clentfort opened this issue Jun 4, 2024 · 4 comments
Labels
🎁 mobx-react-lite Issue or PR related to mobx-react-lite package ❔ question 💀 wontfix

Comments

@clentfort
Copy link

Intended outcome:

npm install puts react-dom into the right directory so that mobx-react-lite can find it.

Actual outcome:

react-dom might be placed nested into the node_modules of another node_module so that if mobx-react-lite is not right next to react-dom it can not be found.

How to reproduce the issue:

tl;dr

  1. Clone https://github.com/clentfort/mobx-react-lite-npm-peer-deps/tree/main
  2. Run npm exec redocly, see Error: Cannot find module 'react-dom' error.

I ran into this issue when trying to use a package that depends on mobx-react-lite (namely @redocly/cli). They have correctly added react-dom as a dependencies in their package.json. However, with a second dependency in play (cdktf-cli) running npm install will put react-dom into node_modules/@redocly/cli/node_modules/react-dom while mobx-react-lite ends up in node_modules/mobx-react-lite. With this node_modules layout mobx-react-lite can't find react-dom. I think mobx-react-lite needs to add react-dom to it's peerDependencies. It is already present in peerDependenciesMeta and flagged as optional, but that doesn't tell npm that react-dom is an actual peerDependency. It's just meta information about a non-exisiting peerDependency.

Versions

mobx-react-lite: 3.4.3
npm: 10.4.0
node: v20.10.0

@mweststrate
Copy link
Member

react-dom is deliberately not part of the deps as it breaks with react-native. However, with normal node resolution rules, that shouldn't matter, as a module should resolve as long as it appears anywhere in node_modules. So make sure react-dom is a dependency of the "host" or top level package.json, or the same package.json that requires mobx-react-lite and you should be fine.

I think the real issue here is that @Redocly puts react-dom in their dependencies instead of peer-dependencies; a project should have only a single version of react and react-dom , which is why you want those always as peer deps in libraries, so that the owning project can "configure" the actual react / react-dom version for everyone.

@mweststrate mweststrate added ❔ question 💀 wontfix 🎁 mobx-react-lite Issue or PR related to mobx-react-lite package and removed 🐛 bug labels Jul 1, 2024
@clentfort
Copy link
Author

redocly provides a binary document generator. It's not some form of lib or component that one includes in their own application. So their approach of depending on a specific version of react is correct.

The peerDeps of mobx-react-lite are currently invisible to any package manager, because they are only listed in the meta but not in the peerDeps themselves. They are marked as optional, so package managers should not install these during a normal add/install. So I'm not sure there would be an actual conflict between native and dom

@mweststrate
Copy link
Member

Ah I see what you are getting at. But as far as I understand with optional it won't be installed either? Would you mind doing a quick test that if mobx-react-lite@4.0.7-alpha does indeed install things correctly?

@clentfort
Copy link
Author

I tested it and unfortunately, you are right and it still can not be found. I did hope that npm will put it into the right location but it didn't. Bummer.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🎁 mobx-react-lite Issue or PR related to mobx-react-lite package ❔ question 💀 wontfix
Projects
None yet
Development

No branches or pull requests

2 participants