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

[Bug?]: React Native monorepo support and yarn 3.1+ #3699

Closed
1 task
renchap opened this issue Nov 4, 2021 · 33 comments
Closed
1 task

[Bug?]: React Native monorepo support and yarn 3.1+ #3699

renchap opened this issue Nov 4, 2021 · 33 comments
Assignees
Labels
bug Something isn't working stale Issues that didn't get attention

Comments

@renchap
Copy link

renchap commented Nov 4, 2021

Self-service

  • I'd be willing to implement a fix

Describe the bug

Yarn 3.1.0 now hoists dependencies of dependencies when using installConfig.hoistingLimits: "workspaces".

This is a new behaviour compared to 3.0.2

This is breaking some setups, in my case with a React Native app in a monorepo. Metro (React Native bundler) expects all of its dependency tree to be in one unique node_module folder.

From the changelog I understand the previous behaviour might have been unwanted and 3.1.0 "optimized" things, but in practice it breaks setups that relied on it.

To reproduce

As this is a behaviour change and not a bug, I am not sure you need a way to easily reproduce it?

Environment

System:
    OS: macOS 12.0.1
    CPU: (8) x64 Intel(R) Core(TM) i7-6700HQ CPU @ 2.60GHz
  Binaries:
    Node: 16.13.0 - /private/var/folders/1b/wb5vmt3x7kg8gcgt4l11wbjm0000gn/T/xfs-981831cb/node
    Yarn: 3.1.0 - /private/var/folders/1b/wb5vmt3x7kg8gcgt4l11wbjm0000gn/T/xfs-981831cb/yarn
    npm: 8.1.0 - /usr/local/Cellar/node@16/16.13.0/bin/npm

Additional context

Should this be a feature request for a new hoistingLimits value for hoisting the whole dependency tree?

@renchap renchap added the bug Something isn't working label Nov 4, 2021
@renchap renchap changed the title [Bug?]: [Bug?]: yarn 3.1.0 no longers hoists the whole dependency tree with hoistingLimits: "workspace" Nov 4, 2021
@danoc
Copy link

danoc commented Nov 10, 2021

Did you mean to use workspaces instead of workspace?

https://yarnpkg.com/configuration/manifest#installConfig.hoistingLimits

@renchap
Copy link
Author

renchap commented Nov 10, 2021

Did you mean to use workspaces instead of workspace?

Sorry thats a mistake in my bug report, I was indeed using workspaces and it worked with yarn 3.0.

@renchap renchap changed the title [Bug?]: yarn 3.1.0 no longers hoists the whole dependency tree with hoistingLimits: "workspace" [Bug?]: yarn 3.1.0 no longers hoists the whole dependency tree with hoistingLimits: "workspaces" Nov 10, 2021
@yarnbot
Copy link
Collaborator

yarnbot commented Dec 10, 2021

Hi! 👋

This issue looks stale, and doesn't feature the reproducible label - which implies that you didn't provide a working reproduction using Sherlock. As a result, it'll be closed in a few days unless a maintainer explicitly vouches for it or you edit your first post to include a formal reproduction (you can use the playground for that).

Note that we require Sherlock reproductions for long-lived issues (rather than standalone git repositories or similar) because we're a small team. Sherlock gives us the ability to check which bugs are still affecting the master branch at any given point, and decreases the amount of code we need to run on our own machines (thus leading to faster bug resolutions). It helps us help you! 😃

If you absolutely cannot reproduce a bug on Sherlock (for example because it's a Windows-only issue), a maintainer will have to manually add the upholded label. Thanks for helping us triaging our repository! 🌟

@yarnbot yarnbot added the stale Issues that didn't get attention label Dec 10, 2021
@yarnbot yarnbot closed this as completed Dec 15, 2021
@larixer
Copy link
Member

larixer commented Dec 16, 2021

@renchap

Yarn 3.1.0 now hoists dependencies of dependencies when using installConfig.hoistingLimits: "workspaces".
This is a new behaviour compared to 3.0.2

Yarn has always hoisted dependencies of dependencies with installConfig.hoistingLimits: "workspaces". If you don't want this, you should rather use installConfig.hoistingLimits: "dependencies"

@renchap
Copy link
Author

renchap commented Dec 16, 2021

@larixer I am currently using 3.0.3 and this is not the case, the whole dependency tree for my package with nstallConfig.hoistingLimits: "workspaces" is in my packages's directory node_modules.

@larixer
Copy link
Member

larixer commented Dec 16, 2021

@renchap Please provide reproduction

@larixer
Copy link
Member

larixer commented Dec 16, 2021

@renchap The behavior for installConfig.hoistingLimits: "workspaces" has never been changed, at least, not intentionally.

@renchap
Copy link
Author

renchap commented Dec 21, 2021

I ran more tests, and it is indeed a bit more complex than what I thought: dependencies of other workspaces are not hoisted when using installConfig.hoistingLimits: "workspaces" starting with 3.1.

Here is a repro: https://github.com/renchap/test-yarn

In this repo, there are 3 workspaces:

  • packages/shared contains @test/shared, with jwt-decode as a dep
  • packages/web contains @test/web , with @test/shared as a dep
  • packages/mobile contains @test/mobile, with @test/shared as a dep and installConfig.hoistingLimits: "workspaces" because it requires it's whole dependency tree in its node_modules

To reproduce:

  • clone the repo
  • run yarn
  • ls -d packages/mobile/node_modules/jwt-decode: the package is hoisted
  • run yarn set-version 3.1.1 && yarn
  • ls -d packages/mobile/node_modules/jwt-decode: the package has not been hoisted

@larixer
Copy link
Member

larixer commented Dec 22, 2021

Cool, many thanks for the repro @renchap, I will take a look and come back!

@larixer
Copy link
Member

larixer commented Dec 22, 2021

@renchap Hm, for me, its the opposite, with Yarn 3.1.1, there is no packages/mobile/node_modules/jwt-decode present. With Yarn < 3.1 the packages/mobile/node_modules/jwt-decode is present. I think Yarn >= 3.1 is right for this case, because jwt-decode is the direct dependency of the @test/shared workspace and it should be accessible from @test/shared workspace. The mobile workspace gets access to the @test/shared workspace through a symlink - Node.js executes realpath on the symlink and then finds the dependencies by traversing directory structure up. Hence it was a bug in Yarn < 3.1 that direct dependencies of the @test/shared ended up in @test/mobile.

The nested workspace hoisting was changed to fix the bug:
#3429
in a pull request:
#3438

@renchap
Copy link
Author

renchap commented Dec 22, 2021

@larixer Sorry, you are right, its the other way around. I should not post issues just before going to sleep.

The issue I am encountering is with the Metro bundler (React Native).
It does not have support for workspaces not symlinks and has its own resolution mechanism, so we are using metro-symlinked-deps to force it resolve all dependencies from its own node_modules.

As Yarn allows us to dont hoist anything for a workspace it worked fine, but now that those packages are there only through a symlink it no longer works. I tried to dig into Metro's code (which depends on some internal jest packages…) but this has been an issue for years and multiple PRs tried to fix this, but it's still an issue.

Do you think there could be a new hoistingLimits value in Yarn to enable this behaviour?

@larixer
Copy link
Member

larixer commented Dec 22, 2021

@renchap I wonder is there a better way to support symlinks with Metro? I think it will be frustrating one way or another if Metro will not resolve workspace dependencies correctly, I cannot imagine the solution when you just provide some clever hoisting and it magically makes React Native resolve dependencies correctly in all the cases. Do you think you can showcase the problem with Metro somewhere on GitHub, so that I tried to better understand the problem and play with it?

@renchap
Copy link
Author

renchap commented Dec 22, 2021

Sure, here are a few issues:

@larixer
Copy link
Member

larixer commented Dec 22, 2021

@renchap Yes, I understand, I cannot solve the problem on React Native side, but in a specific example Yarn 3+, React Native project I think it is possible to find the solution how to properly configure Metro, hopefully with some set of plugins. To seek the solution I need something to start with some toy project that showcases the problem.

@renchap
Copy link
Author

renchap commented Dec 22, 2021

Do you want me to prepare a basic repo with a RN workspace showcasing the issue?

@larixer
Copy link
Member

larixer commented Dec 22, 2021

Do you want me to prepare a basic repo with a RN workspace showcasing the issue?

Yep, that would be awesome if you can and have time!

@larixer
Copy link
Member

larixer commented Dec 22, 2021

@renchap Do you think it will solve the problem, if we have something like hoistingLimits: react-native, which will make the pre Yarn 3.1 hoisting behavior for the workspace it is declared on, i.e. it will inject all used workspaces dependencies into the mobile workspace and hoist them?

@larixer larixer removed the stale Issues that didn't get attention label Dec 22, 2021
@larixer larixer reopened this Dec 22, 2021
@larixer larixer self-assigned this Dec 22, 2021
@larixer larixer changed the title [Bug?]: yarn 3.1.0 no longers hoists the whole dependency tree with hoistingLimits: "workspaces" [Bug?]: React Native monorepo support and yarn 3.1+ Dec 22, 2021
@larixer
Copy link
Member

larixer commented Dec 22, 2021

@renchap I'm evaluating different options about React Native monorepo support . I wonder, how Expo seems to be able to support monorepo, as they seems to be React Native-based, did they patch React Native module resolution code?

@renchap
Copy link
Author

renchap commented Dec 22, 2021

Do you think it will solve the problem, if we have something like hoistingLimits: react-native, which will make the pre Yarn 3.1 hoisting behavior for the workspace it is declared on, i.e. it will inject all used workspaces dependencies into the mobile workspace and hoist them?

Yes, it would at least for me, as I am using a metro plugin to force metro to use the local node_modules for everything. I dont know if it would be enough for a base RN app, I would need to test it.

I wonder, how Expo seems to be able to support monorepo, as they seems to be React Native-based, did they patch React Native module resolution code?

I have not used Expo so I dont really know.

As a side note, it would be awesome to be able to use Yarn PNP with RN as it currently prevents us to use it entirely (everything in a monorepo, including the RN apps), but this is maybe another topic entirely (as many packages needs to be unplugged for the native code parts to find the files (see react-native-community/cli#27 for example).

@larixer
Copy link
Member

larixer commented Dec 22, 2021

I have not used Expo so I dont really know.

I mean, they declare that they support Yarn workspaces (without any tweaks to hoisting). They seem to do it by purely configuring Metro. I wonder maybe it's possible to utilize their work in a vanilla React Native application by using Expo's Metro config as a base, something like this:
https://stackoverflow.com/questions/64802707/expo-monorepo-with-expo-yarn-workspaces-how-to-customize-the-metro-config-js-fi
Note, you don't need yarn-expo-workspaces with the latest @expo/metro-config

As a side note, it would be awesome to be able to use Yarn PNP with RN as it currently prevents us to use it entirely (everything in a monorepo, including the RN apps), but this is maybe another topic entirely (as many packages needs to be unplugged for the native code parts to find the files (see react-native-community/cli#27 for example).

Yes, it would be interesting to make RN work with Yarn PnP, but its worth a separate issue.

@cm-nutrien
Copy link

Note, you don't need yarn-expo-workspaces with the latest @expo/metro-config

Wait, can you explain that? How come yarn-expo-workspaces isn't needed now?

@larixer
Copy link
Member

larixer commented Dec 28, 2021

@cm-nutrien Check out the official Expo docs for configuring monorepos, they neither use nohoist nor yarn-expo-workspaces since Expo SDK 43:
https://docs.expo.dev/guides/monorepos/

@cm-nutrien
Copy link

Somehow I'd completely missed that, thank you for pointing that out!

@yarnbot
Copy link
Collaborator

yarnbot commented Jan 27, 2022

Hi! 👋

This issue looks stale, and doesn't feature the reproducible label - which implies that you didn't provide a working reproduction using Sherlock. As a result, it'll be closed in a few days unless a maintainer explicitly vouches for it or you edit your first post to include a formal reproduction (you can use the playground for that).

Note that we require Sherlock reproductions for long-lived issues (rather than standalone git repositories or similar) because we're a small team. Sherlock gives us the ability to check which bugs are still affecting the master branch at any given point, and decreases the amount of code we need to run on our own machines (thus leading to faster bug resolutions). It helps us help you! 😃

If you absolutely cannot reproduce a bug on Sherlock (for example because it's a Windows-only issue), a maintainer will have to manually add the upholded label. Thanks for helping us triaging our repository! 🌟

@yarnbot yarnbot added the stale Issues that didn't get attention label Jan 27, 2022
@renchap
Copy link
Author

renchap commented Jan 27, 2022

Sorry for not having posted an update here!
I still plan to test expo's cli Metro config and write some doc on how to use it on non-expo packages (or publish a module to do it without installing expo) once I got time to test it!

@renchap
Copy link
Author

renchap commented Feb 3, 2022

I have been able to test the monorepo instructions on this page: https://docs.expo.dev/guides/monorepos/

  • use expo/metro-config
  • updating XCode's config to look at the correct node_packages folder (in the root of the repo) for the various build commands
  • add export PROJECT_ROOT=$PWD in �the "Bundle React Native code and images" build step
  • add --no-jetifier to the android build command (Jetifier should no longer be needed on modern apps)

This allowed me to build my React Native app without the need for hoisting and with the latest yarn version 🎉

@renchap renchap closed this as completed Feb 3, 2022
@joeyfigaro
Copy link

Any approaches for those of us not using expo and are running into issues with yarn 3+?

@renchap
Copy link
Author

renchap commented Feb 11, 2022

You should be able to look at how expo/metro-config configures Metro and copy it in your own config, I dont think they do anything specific to Metro here.

@jmarek-sky
Copy link

@renchap I wonder if you were able to use the Yarn's PnP feature with your setup?
I got our RN monorepo more less working with Yarn 3, but only in the legacy "node_modules" linker mode. Were you successful somehow working around RN cli not supporting the PnP yet?

@renchap
Copy link
Author

renchap commented May 6, 2022

@jmarek-sky No, I did not manage to use PnP with React Native, we are still using the node_modules linker.

@serhiipalash
Copy link

I have been able to test the monorepo instructions on this page: https://docs.expo.dev/guides/monorepos/

@renchap could you be so kind as to share the base monorepo you did? :)

I am working on the same problem at the moment.

@renchap
Copy link
Author

renchap commented Aug 31, 2022

I recently switched to https://microsoft.github.io/rnx-kit/ and it handles a yarn 3 monorepo (with node_modules linker) correctly.

Mostly due to https://microsoft.github.io/rnx-kit/docs/tools/metro-resolver-symlinks

@serhiipalash
Copy link

I see. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working stale Issues that didn't get attention
Projects
None yet
Development

No branches or pull requests

8 participants