Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Dependencies, that have react-native as a peerDependency, resolve to the wrong version of react-native #22

Open
todorone opened this issue Oct 18, 2021 · 21 comments
Labels
good first issue Good for newcomers

Comments

@todorone
Copy link

Thanks for such a great monorepo setup

It works great out of the box, but as soon as i started to add dependencies, that use react-native, it stopped working for me.

I tried all combination of nohoist and resolutions, but i was not able to set version of nested react-native dependency of dependencies of platforms. Any hints?

@mmazzarolo
Copy link
Owner

Hi @todorone !
Can't play with reanimated now, but did you already mark it as nohoist and ran yarn reset?
As long as both react-native and reanimated are in the right node_modules (so in the package-level node_modules) they should resolve to the right peer-dependency.

@todorone
Copy link
Author

todorone commented Oct 18, 2021

Thanks for reply, @mmazzarolo

I marked it as nohoist(with reset), tried to set the correct version with with resolutions property, no luck. Will try to assemble minimal repro tomorrow.

@mmazzarolo
Copy link
Owner

mmazzarolo commented Oct 18, 2021

Why are you marking it in the resolutions, it shouldn't be required (and from my experience it's even detrimental :/)?
Could you verify that reanimated and react-native are being added to the right node-modules?

@likern
Copy link

likern commented Oct 22, 2021

@mmazzarolo Does monorepo tools supports RN 0.66.1 version?

@likern
Copy link

likern commented Oct 22, 2021

@mmazzarolo I think I can confirm this bug. I use example applications with different RN versions (0.66.0 and 0.66.1). You can see code here breeffy/react-native-monorepo#76.

When I added 0.66.1 I see similar to this bug facebook/react-native#31512.

Moreover in my applications I use strict versions:
app with 0.66.0 RN version

dependencies: {
  "react-native-svg": "=12.2.0"
}

app with 0.66.1 RN version

dependencies: {
  "react-native-svg": "=12.2.0"
}

In 0.66.0 node_modules/react-native-svg/node_modules/ is empty (and it should resolve to upper node_modules).
In 0.66.1 node_modules/react-native-svg/node_modules/ contains invalid react-native

[mobile-0.66.1]$ ls node_modules/react-native-svg/node_modules/
react-native //  <--- 0.66.0 !!!!!

I think the reason is that resolution algorithm sees that workspace occupied by invalid RN version and have no choice, but installed it into node_modules/ of react-native-svg. I think intermediate node_modules (which are upper in hierarhy, but not yet workspace are ignored).

Thus react-native is installed into react-native-svg's node_modules/.
Because it has package.json with

  "peerDependencies": {
    "react": "*",
    "react-native": ">=0.50.0"
  }

it can rightfuly install 0.66.0 version. What yarn does.

@likern
Copy link

likern commented Oct 22, 2021

As soon as I removed react-native folder in node_modules/react-native-svg/node_modules/ react-native-svg starts seeing correct react-native version and I can run application.

├─┬ [email protected]
│ └── [email protected] deduped
└─┬ [email protected]

@todorone
Copy link
Author

@mmazzarolo Sorry for the delay, i put the simple repro - https://github.com/todorone/react-native-universal-monorepo with one simple commit of adding react-native-reanimated(but it's relevant to any dependency that depends on react-native and needs to be nohoisted)

Result of running yarn:
mobile/node_modules/react-native-reanimated/node_modules/react-native/package.json // 0.65.1 - is ok
tv/node_modules/react-native-reanimated/node_modules/react-native/package.json // 0.65.1 - is not ok, as we use a custom fork of RN 0.64.2 so it breaks Metro bundler

Please let me know if any further clarification is needed and thanks again for working on such a cool monorepo setup.

@likern
Copy link

likern commented Oct 23, 2021

@todorone Looks like this is exactly problem I described above. I briefly looks at solving this problem.
And I'm sure there is no solution (hope to be wrong).

The only way to fix this is to use postinstall script which will delete all react-native packages under nested node_modules - this will require node to look react-native in above folders, thus it could find correct version.

@mmazzarolo
Copy link
Owner

mmazzarolo commented Oct 23, 2021

Thanks for reporting all, these info are super helpful 👍

I'm gonna give it a try soon.

Just FYI, @todorone :

but it's relevant to any dependency that depends on react-native and needs to be nohoisted)

Unless something changed in yarn or in the new react-native version, I'm pretty sure this is something that can be solved with some tweaking. It's not happening on all libs that depend on react-native (see @react-native-async-storage, for example, which is part of this repo). I'm wondering if the libraries you're using depend on other native libraries as well, which sometimes break yarn resolution algorithm — this happened a few times to me in the past and I solved it by adding the nested libraries to the nohoist as well (e.g., with **/react-native-reanimated, **/react-native-reanimated/**).

Also, just to clarify: react-native-monorepo-tools (currently) doesn't update/move packages around, so this would be something happening at the yarn level.

I'll give it a try soon and get back to you.

@mmazzarolo
Copy link
Owner

Here are my initial research results.
I tried installing react-native-reanimated, and I can replicate the issue.
For now, I'm pretty sure the issue is happening because of this known (and unsolved) Yarn bug: yarnpkg/yarn#5520. I've already encountered it in the past, and it seems to happen because Yarn Classic gets confused in some cases when dependencies (like react-native-reanimated) have peerDependencies with some specific samever ranges (like *), and it ends up installing them in the dependency's node_modules. Additionally, Yarn Classic installs peerDependencies by default (which shouldn't be correct IMHO, but that's a different story).
In my other projects where I'm using this approach I'm using a postInstall script (like @likern suggested, which btw, is what Expo doe too) to fix the symlinks where needed. I haven't shared/published it because while working on this monorepo I was able to install several dependencies that had peerDependencies on React and React-Native (like @react-native-async-storage), so I assumed the Yarn bug was fixed... but looks like I was wrong, lol.

FYI, there are many other peer-dependencies related bugs in Yarn Classic (yarnpkg/yarn#8113, yarnpkg/yarn#8606, yarnpkg/yarn#6483, yarnpkg/yarn#7486, yarnpkg/yarn#7572, and this).

I see a few options to solve this issue in your cases and in the repo (in order of elegance, IMHO):

  • Update react-native-monorepo-tools to block react-native-reanimated/node_modules/react-native (or from react-native-svg, which is what I just tried). From my testing, just adding something like blockList.push(new RegExp(node_modules/react-native-svg/node_modules/react-native/.*)); at line 46 here should be enough (you can try editing it directly in your node_modules/react-native-monorepo-tools and restarting the metro server). Of course it'd need to be made a bit more dynamic (e.g., by applying this rule to all nohoisted libs) but it can be a starting point.
  • Update react-native-monorepo-tools adding a post-install script that clean-ups the wrongly installed react-native instances.
  • Move to Yarn 2. The more I stay in Yarn Classic, the more I see issues like these popping up that won't be solved in Yarn Classic. Not a fan of it, but if anyone wanna try tackling it on a separate branch I'm happy to add it to the docs.
  • Nohoist everything (with **). Consumes more space on disk, but unless you're targeting all platforms it should be ok.

If you could play a bit with the first option and return with some feedback, that would be great 👍 (you might also wanna block react from that dir too). If it works, we can start thinking about how to add it to the monorepo tools repo in an elegant way.

image

image

@likern
Copy link

likern commented Oct 23, 2021

@mmazzarolo Thanks for deep research. I've tried to use pnpm, but it didn't work at all. Also I couldn't make it nohoist react-native. Not suitable for React Native project.

Then I tried yarn v3 and had great success 🚀 😮
Снимок экрана от 2021-10-23 21-34-03

Inside example application I see all dependencies to react-native are resolved correctly.
For React Native 0.66.1
Снимок экрана от 2021-10-23 21-35-14

For React Native 0.66.0
Снимок экрана от 2021-10-23 21-37-00

You can see commits adding yarn v3 here breeffy/react-native-monorepo#76

Everything is working 🤟🏻 I can run example application under different React Native versions simultaneously.

Also my monorepo packages are linked correctly by yarn (I see links to packages).
Снимок экрана от 2021-10-23 22-21-50

I use quite a lot of native dependencies including react-native-reanimated, react-native-svg, react-native-gesture-handler so I think this approach passed test.

    "nohoist": [
      "**/react",
      "**/react-dom",
      "**/react-native",
      "**/react-native/**",
      "**/react-native-web",
      "**/react-native-svg",
      "**/react-native-reanimated",
      "**/react-native-gesture-handler",
      "**/react-native-animateable-text",
      "**/@react-navigation/native",
      "**/@react-navigation/native-stack",
      "**/react-native-screens",
      "**/react-native-safe-area-context",
      "**/@react-native-async-storage/async-storage"
    ]

@likern
Copy link

likern commented Oct 23, 2021

From my research the only symlinks I have under node_modules in example application are my monorepo packages.
Снимок экрана от 2021-10-23 22-21-50

So the question is do we need to keep all these nohoist things (since they only now used by metro.config.js)?
Since all these nohoist packages are already directly copied to node_modules/ by yarn v3 automatically.

I think we can remove most of metro.config.js changes if not all. For my setup we only need to resolve symlinked monorepo packages (and it's dependencies?)

For the original article (where different RN are used) I think only shared example-app need to be resolved.

@mmazzarolo
Copy link
Owner

mmazzarolo commented Oct 23, 2021

I'll check it tomorrow, but the idea here is to continue supporting Yarn Classic as well.
As mentioned/shown above, it can be done with a few tweaks on the metro side, and it will be compatible with all Yarn versions 👍
It's definitely worth mentioning in the README that using Yarn 2+ solves several issues — but it's quite a huge jump for many users, so updating the metro config should be done either way IMHO (I'll take care of it as soon as I have some more free time).
Thanks so much for this discussion and for your comments btw, they are super helpful!

@likern
Copy link

likern commented Oct 23, 2021

It will be compatible in a way that using yarn classic will work. Globally installed yarn (no matter version) automatically runs per-project yarn (no matter version). It's the most bulletproof approach.
For yarn classic it's outlined here https://classic.yarnpkg.com/en/docs/cli/policies#enforcing-yarns-version-across-your-project-

Yarn v3 is using per-project setup (yarn classic also supports this). So global yarn will always pick up correct version from <project>/.yarn/releases. No need to update it.

@mmazzarolo
Copy link
Owner

mmazzarolo commented Oct 24, 2021

It will be compatible in a way that using yarn classic will work. Globally installed yarn (no matter version) automatically runs per-project yarn (no matter version). It's the most bulletproof approach.

Not really? I mean, you'd still need to have Yarn 2+ installed to make this repo work. I wouldn't define it as "bulletproof" if we force people to have Yarn 2+ installed when we can fix it on our side (on react-native-monorepo-tools) and make it work on both Yarn Classic and Yarn 2+.

To clarify: I don't have anything against Yarn 2+ and/or Yarn 2+ setups. I'm using it on my machine, I like it, and I know it's backward compatible 👍
Still, it has (and/or just recently fixed) a few quirks (e.g., some libraries just recently became fully compatible with it, like Cypress/Puppeteer), and it's not widely adopted (yet).
Yarn Classic, on the other hand, is still very popular. Yes, it still has several (well-known) issues, but you can make it work with a React Native monorepo with some tweaks. This is why I think fixing it on the monorepo-build-tools side is worth it.
I'm not a fan of adding build-tools on top of build-tools, but we need to face the fact that the entirety of this project (and of all other projects that brings monorepo support to React Native codebases) goal is to patch build-tools so support a React Native monorepo.
If Metro + Yarn Classic/NPM (which are still the most widely adopted React Native build tools) were compatible with React Native monorepos out-of-the-box, there wouldn't be any need for tutorials and repos such as this one 👍

Let me know if I'm missing anything 👍

@mmazzarolo mmazzarolo changed the title Dependencies, that have react-native as a dependency, resolve to one version of react-native Dependencies, that have react-native as a peerDependency, resolve to one version of react-native Oct 24, 2021
@mmazzarolo mmazzarolo changed the title Dependencies, that have react-native as a peerDependency, resolve to one version of react-native Dependencies, that have react-native as a peerDependency, resolve to the wrong version of react-native Oct 24, 2021
@todorone
Copy link
Author

I completely agree with @mmazzarolo that the best option is to do the best to maintain compatibility with Yarn classic if it's possible. There is a reason why Yarn 2+ has low adoption - there was a pretty heated discussion between the community(including Facebook and some other companies employees) and the current Yarn maintainer, which did not want to follow propositions of maintaining compatibility with the classic Yarn.

In the case it's not possible at all, switching to the new Yarn is also the way, but there will be serious friction about it...

@likern
Copy link

likern commented Oct 24, 2021

There is a compatibility issues between yarn classic versions. For RN monorepo I had to switch to older version (1.22.* or so didn't work) because of some issue (don't remember details).

So to ensure consistency (users might have different yarn classic versions installed) I embedded yarn code into project itself.

No matter of what yarn version installed globally on user machine, as soon as you go to project directory yarn --version will print always correct stable version, specific to project. It's done automatically as it's a yarn feature.

So global yarn just redirects commands to yarn which located inside repository.

@likern
Copy link

likern commented Oct 24, 2021

So I'm not against proposed solution. Just wanted to mention there's that yarn feature exists (in yarn classic too).

And to say that supporting yarn classic is not enough.
If that yarn feature is applied https://classic.yarnpkg.com/en/docs/cli/policies/ it's no matter what yarn version is on user machine.

Repository will always use correct (valid / tested) yarn version, no matter is it yarn classic or yarn v3.

So repository can use yarn v3 and get all goodies for free, without compromising developer experience.

@serge-sky
Copy link

here is nice and easy metro.config alternative to work with yarn v1.
https://gist.githubusercontent.com/Titozzz/66a5fce69cfbf93e488350551454aa7c/raw/b689adb35310fb5bb09d501fe18c4d235b6c091e/metro.config.js

Took from this article https://engineering.brigad.co/react-native-monorepos-code-sharing-f6c08172b417

@mmazzarolo
Copy link
Owner

@serge-sky , yeah, seems similar to the fix I mentioned above 👍 PRs to the monorepo tools are welcome!

@likern
Copy link

likern commented Dec 9, 2021

Share my experience with yarn v3. It works great, everything is smooth, even consuming native dependencies. Until today I was very happy.

But today I tried to patch native dependency which uses JSI C++ code https://github.com/ospfranco/react-native-quick-sqlite and it's completely broken.

Native dependencies require exact package location (since they require native parts in C++ using "../../react-native/..." paths).
Symlinking them makes packages broken (they can't find react-native using these paths since packages are symlinked).
And Yarn v3 gives no control on how to fix this.

https://github.com/ospfranco/react-native-quick-sqlite/blob/e736497166481bbb14a6a9da1df6691ccbee1204/android/CMakeLists.txt#L37

But the story is even worse 😢 I can't even switch back to yarn classic for the exact repository.
Even if I directly clone [email protected] by npm pack and run node ./yarn.js -- --version it gives incorrect (globally installed) version 3.0.2. So yarn v3 completely broke everything in that regard. Of course, neither I can easily switch back to yarn classic completely 😠 (since using yarn v3 features).

I just can't now develop native code (the happy part it's not mandatory for me).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants