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

Codegen on iOS is failing in monorepo projects #35429

Closed
byCedric opened this issue Nov 22, 2022 · 6 comments
Closed

Codegen on iOS is failing in monorepo projects #35429

byCedric opened this issue Nov 22, 2022 · 6 comments
Labels

Comments

@byCedric
Copy link
Contributor

byCedric commented Nov 22, 2022

Description

As mentioned in the partner sync, the current codegen scripts contain hardcoded paths which are incorrect in monorepo projects.

To demonstrate the issue, I updated my Expo monorepo example repository. You can see a failed build in this PR with the relevant xcode logging pointing to these hardcoded paths.

Note that Android does work fine in the example repo, but I don't think we are using it. I can also see other hard-coded paths from the template.

Core issue

In React Native's native files, we use quite some scripts to generate certain things. Paths to these scripts are often hard coded. These hardcoded paths are fine, but only within the react-native package.

When hardcoding paths to other packages, like react-native-codegen, you could easily break general monorepo support. Some monorepo tools allow people to mark certain packages as "do not hoist". While this works for some cases, it's often not the best usage of monorepos as this would duplicate installations in bigger repos and consume more disk space. With that in mind, we basically can't assume the location of a package, we have have to ask Node to resolve it.

Issue from the example repo

The linked example repository uses a relatively simple structure listed below. We hoist as many packages as possible to speed up installation and consume minimal disk space.

Expo monorepo
├── apps
│   └── mobile
│       └── node_modules
│           └── [email protected]
└── node_modules
    └── [email protected]

In this case, the path to react native codegen is incorrect, as the monorepo tool (pnpm) is installing codegen in the root node_modules folder.

Looking to npm & future

We might run into more issues in the future when the npm isolation mode RFC is accepted. It's probably worth it to think of solutions before that.

From Expo SDK 43, we switched these hard-coded paths to node module resolution references to better accommodate monorepos and custom setups. (android example, ios example)

Using a similar but slightly different solution could even help make React Native compatible with Plug'n'Play modules. Instead of using hard-coded paths, and "expect files to be installed on disk", we could ask Node or the package manager to resolve the file locations. If all of these requests go through a Plug'n'Play manager, it could download those files on the fly, and return the location. The first run will be slow, but IMHO it's worth investigating.

Version

0.70.5

Output of npx react-native info

System:
    OS: macOS 12.6
    CPU: (8) x64 Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
    Memory: 880.21 MB / 32.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.18.0 - ~/.nvm/versions/node/v16.18.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v16.18.0/bin/yarn
    npm: 8.19.2 - ~/.nvm/versions/node/v16.18.0/bin/npm
    Watchman: Not Found
  Managers:
    CocoaPods: 1.11.2 - /usr/local/bin/pod
  SDKs:
    iOS SDK:
      Platforms: DriverKit 22.1, iOS 16.1, macOS 13.0, tvOS 16.1, watchOS 9.1
    Android SDK:
      API Levels: 21, 26, 28, 29, 30, 31
      Build Tools: 26.0.3, 28.0.0, 28.0.1, 28.0.2, 28.0.3, 29.0.0, 29.0.1, 29.0.2, 29.0.3, 30.0.0, 30.0.1, 30.0.2, 30.0.3, 31.0.0, 31.0.0
      System Images: android-21 | Google APIs ARM EABI v7a, android-22 | Intel x86 Atom_64, android-22 | Google APIs ARM EABI v7a, android-22 | Google APIs Intel x86 Atom_64, android-23 | Intel x86 Atom_64, android-26 | Google APIs Intel x86 Atom_64, android-28 | Google Play Intel x86 Atom, android-29 | Google Play Intel x86 Atom, android-30 | Google Play Intel x86 Atom
      Android NDK: Not Found
  IDEs:
    Android Studio: 2021.2 AI-212.5712.43.2112.8815526
    Xcode: 14.1/14B47b - /usr/bin/xcodebuild
  Languages:
    Java: 18.0.2 - /usr/local/opt/openjdk/bin/javac
  npmPackages:
    @react-native-community/cli: Not Found
    react: Not Found
    react-native: 0.70.5 => 0.70.5 
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

Steps to reproduce

To trigger the issue:

  • $ git clone [email protected]:byCedric/expo-monorepo-example.git ./example
  • $ cd ./example
  • $ git checkout 0c94b0aa06e95a3ac9dcb87aff3bf3e3f767d7ac (this is the latest on main without a patch)
  • $ pnpm install
  • $ cd apps/mobile
  • $ npx expo prebuild --platform ios
  • $ xed ios
  • Build the app, should fail

To test the fix from PR #73

  • $ git clone [email protected]:byCedric/expo-monorepo-example.git ./example
  • $ cd ./example
  • $ git checkout fix/codegen-monorepo-paths (this is the latest on main without a patch)
  • $ pnpm install
  • $ cd apps/mobile
  • $ npx expo prebuild --platform ios
  • $ xed ios
  • Build the app, should succeed

Snack, code example, screenshot, or link to a repository

https://github.com/byCedric/expo-monorepo-example

@peterpme
Copy link

Great writeup 💯

FWIW also happens with yarn

@byCedric
Copy link
Contributor Author

byCedric commented Nov 23, 2022

As a follow up, I found this piece of code that re-implements Node module resolution in Android. Implemented by no other than @ide 😄 It's nice, but I don't think this would work for isolated modules.

Unfortunately, it still requires an additional path for this monorepo. That shouldn't be necessary, since react-native-codegen is technically part of a workspace in this repository (and thus should have been detected by findNodeModulePath).

That said, I'm not sure why you'd want to install from source without running yarn install first, it seems like a hard requirement and maybe not worth the trouble of hardcoding "backup paths".

@byCedric
Copy link
Contributor Author

byCedric commented Nov 23, 2022

The reason why I stress the isolated modules part is because Metro seems to be working on supporting the default pnpm behavior. pnpm is one of the few package managers that uses this isolated modules mode by default, unless using node-linker=hoisted (specifically mentioned for React Native projects in pnpm docs). But, in order to fully use this structure, the native layer must know how to locate these packages.

A short summary of isolated modules is that only the direct dependencies are installed in the node_modules where they are required.

Project #1 - with default node_modules
├── android
├── ios
├── node_modules
│   ├── react
│   ├── react-native
│   └── react-native-codegen
└── package.json
    ├── [email protected]
    └── [email protected]

---

Project #2 - with isolated modules
├── android
├── ios
├── node_modules
│   ├── react
│   └── react-native
│       └── node_modules
│           └── react-native-codegen
└── package.json
    ├── [email protected]
    └── [email protected]

In the 1st example above, the project's direct dependencies are [email protected] and [email protected]. But, because [email protected] has a dependency on [email protected], the package manager installs that module to the node_modules folder.

Now in the 2nd example, only the project's direct dependencies are installed in the project node_modules folder. And the [email protected] dependency is installed in a node_modules folder inside [email protected].

This means that "just iterating from a nested folder, checking each parent folder for node_modules/<pkg> doesn't work anymore in isolated mode. Instead, we'd have to resolve react-native first, and use that location to resolve react-native-codegen.

Just adding this here to give you a good context on what kind of fix is required to make this "properly" work with cutting edge package managers.

@cortinico
Copy link
Contributor

Just adding this here to give you a good context on what kind of fix is required to make this "properly" work with cutting edge package managers.

Thanks for sharing.

I can answer for Android as you called it out here:

Note that Android does work fine in the example repo, but I don't think we are using it. I can also see other hard-coded paths from the template.

Please note that you're referencing a comment. The Android template allows you to customize where the react-native and the react-native-codegen packages will be placed exactly for this reason. It defaults to ../node_modules/react-native-codegen but you should be able to override it to handle your monorepo setup.

For iOS: #35430 should do the fix right?

@byCedric
Copy link
Contributor Author

byCedric commented Nov 23, 2022

@cortinico, that makes sense, thanks. Might be worth defaulting in the templates to non-hardcoded paths that work out of the box in most environments, though :)

I can confirm that PR #35430 does fix the issue in my monorepo templates, including the repo I linked - made a patch to apply this change.

Is it worth to make an RFC to discuss supporting node module resolutions, isolated mode, and possibly plug and play (with pnpapi)?

@cortinico
Copy link
Contributor

Might be worth defaulting in the templates to non-hardcoded paths that work out of the box in most environments, though :)

What would be the suggested path for the codegen that works out of the box in most cases?

Is it worth to make an RFC to discuss supporting node module resolutions, isolated mode, and possibly plug and play (with pnpapi)?

Yes it is 👍 or at least let's move the discussion to discussions-and-proposal repo so more people can participate over there.

kelset pushed a commit that referenced this issue Nov 30, 2022
Summary:
Fixes #35429

This fix is fairly straightforward. Instead of hardcoding two separate paths for this repo or a simple user's project, we ask Node to resolve `react-native-codegen`.

Because `react-native-codegen` is moved [into the `/packages/*` folder](https://github.com/facebook/react-native/blob/main/package.json#L104), it's part of a workspace in this repository. That means that this fix works not only for monorepos in general but also resolves the right path within the react native repository.

You can test this out yourself when running this command in either the root, or any other workspace in this repository:
```bash
node --print "require('path').dirname(require.resolve('react-native-codegen/package.json'))"
```

I've tested this on Node 16 and 18, and seems to work nicely. Note that you _**have to specify `react-native-codegen/package.json`**_. That's because the `react-native-codegen` package itself is technically invalid; it's missing an entry point within the package (no `main`). When running `node --print "require.resolve('react-native-codegen')"` Node can't resolve the main entry point, and will fail during this process.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[iOS] [Fixed] - Fix incorrect codegen CLI paths in monorepo projects

Pull Request resolved: #35430

Test Plan: See PR #35429

Reviewed By: cortinico

Differential Revision: D41475878

Pulled By: cipolleschi

fbshipit-source-id: f0c362b64cf9c3543a3a031d7eaf302c1314e3f0
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this issue May 22, 2023
Summary:
Fixes facebook#35429

This fix is fairly straightforward. Instead of hardcoding two separate paths for this repo or a simple user's project, we ask Node to resolve `react-native-codegen`.

Because `react-native-codegen` is moved [into the `/packages/*` folder](https://github.com/facebook/react-native/blob/main/package.json#L104), it's part of a workspace in this repository. That means that this fix works not only for monorepos in general but also resolves the right path within the react native repository.

You can test this out yourself when running this command in either the root, or any other workspace in this repository:
```bash
node --print "require('path').dirname(require.resolve('react-native-codegen/package.json'))"
```

I've tested this on Node 16 and 18, and seems to work nicely. Note that you _**have to specify `react-native-codegen/package.json`**_. That's because the `react-native-codegen` package itself is technically invalid; it's missing an entry point within the package (no `main`). When running `node --print "require.resolve('react-native-codegen')"` Node can't resolve the main entry point, and will fail during this process.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[iOS] [Fixed] - Fix incorrect codegen CLI paths in monorepo projects

Pull Request resolved: facebook#35430

Test Plan: See PR facebook#35429

Reviewed By: cortinico

Differential Revision: D41475878

Pulled By: cipolleschi

fbshipit-source-id: f0c362b64cf9c3543a3a031d7eaf302c1314e3f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants