search

facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
119.34k stars 24.36k forks source link

Codegen resolves wrong dependency in multi-app monorepo; when apps specify different versions of same dep (ios) #46196

Closed superguineapig closed 2 months ago

superguineapig commented 2 months ago

Description

Abstract

It appears that codegen is improperly resolving (node_module) dependencies when the app's source is part of a multi-app monorepo, and there are multiple versions of the dep specified across apps. This is possibly due to the use of node's require.resolve mechanism.

Background

I have recently migrated a pair of react-native apps into a (yarn workspaces) monorepo setup. While testing one of the apps, I noticed some new warning messages in the terminal during a routine pod install. After checking the dependencies, I verified that the app in question had specified the latest version of the library being flagged. I verified that the dep existed and was updated by the author to fix this exact warning. Poking around more, I noticed that the other app in my repo was still using the older version of the library.

The differences between the two apps was simply the version of the dependency (as specified in their respective package.json) and each dep's (node_modules) location within the monorepo. The older version lived in node_modules at the root, the newer version lived in node_modules adjacent to the app being pod install'ed.

The presence of the codegen deprecation warning indicated that the wrong version of the dependency was being included/discovered during the process for this app (which specifies the appropriate version in its package.json)

TL;DR

Codegen is finding the wrong path to dependencies in this particular monorepo setup

Setup

The repository is set up as follows (note: node deps installed automatically via yarn):

/ (root)
  packages/
    package.json <- no deps specified at root; just yarn workspace config
    node_modules/
      somedep@older version
    App1/
      package.json <- specifies older version of somedep
    App2/
      package.json <- specifies latest version of somedep
      node_modules/ <- yarn added local node_modules with latest version of dep here
        somedep@latest

A pod install from /packages/App2/ios will output a warning message indicating the deprecation of the dependency in question. See below for output.

See below for additional details and a reproducer repo.

Findings

I wasn't super familiar with the chain of events that occurs during a pod install, so it took a while to trace down, but surprisingly, it appears that most of the machinery involved does find the appropriate (paths to) the package dependencies within this monorepo structure (my private repo with lots of stuff going on). Codegen seems to be the outlier.

I traced things down to findExternalLibraries in generate-artifacts-executor.js

This appears to be the code that is responsible for iterating over the apps dependencies and resolving paths to their respective package.json files. It is currently using Node's require.resolve machinery to locate the nearest matching package. However, require will change its search paths based on the how the module itself was imported (module.paths) regardless of the process.cwd(). This makes it non-deterministic when being invoked through a complicated process chain as exhibited by the cocoapods setup.

In a nutshell, the pods executable launches a process which then executes some ruby code, which then requires additional ruby files, eventually executing the Podfile, which then requires more ruby scripts, which may themselves launch additional processes (e.g. node) which then may launch additional processes, and requires, etc, etc, etc. Through that whole chain, it is possible to change the search order for require.resolve (via the spawned processes) even if the process is executing in the expected place.

(Aside: the use of the "command" pattern in the js scripts may be what's messing with the node resolver; not a bug, per se, but just a guess as to the unreliability of resolve in this instance...)

It appears that in this particular repository structure, due to the require search paths, the app local node_modules will not be found. Instead, the search moves down from the location of the generate-artifacts-executor.js script itself, thereby finding the (root-level) node-modules first (where the react-native framework code lives) (in the context of the codegen process; other code (js/rb) seems to resolve things fine)

I came to that conclusion by tracing out the whole chain starting in the Podfile with logging to see what was being found by the various scripts (js/rb) and their functions in the stack; injecting logs of node's module and process state, paths found for each dep, etc

A Potential Fix

Through looking at other react-native js code, I discovered the use of the find-up package in some places. I believe that this can also be used in this instance to invert the search process outward from a deterministic starting point (the project root as opposed to the framework root).

And with very little change required.

The main change would be in findExternalLibraries at line 241

FROM:

...
try {
      const configFilePath = require.resolve(
        path.join(dependency, 'package.json'),
      );
      const configFile = JSON.parse(fs.readFileSync(configFilePath));
...

TO:

...
try {
      const configFilePath = findUp.sync( // <-- using find up instead of require.resolve
        path.join('node_modules', dependency, 'package.json'), // <-- need to prepend 'node_modules'
        {cwd: projectRoot}, // <-- set starting dir to always be relative to project/app
      );
      const configFile = JSON.parse(fs.readFileSync(configFilePath)); // unchanged
...

Some misc changes were also needed to support this:

// Add require at top
const findUp = require('find-up'); // note: this lib is already being used by other RN scripts as well
...

Around Line 514 or so:

// pass projectRoot to call of findExternalLibraries
// within the findCodegenEnabledLibraries function
...
return [
      ...projectLibraries,
      ...findExternalLibraries(pkgJson, projectRoot), // <-- add projectRoot as 2nd arg here
      ...findLibrariesFromReactNativeConfig(projectRoot),
    ];
...

Add projectRoot parameter to findExternalLibraries declaration (Line 228):

...
function findExternalLibraries(pkgJson, projectRoot) { // <-- projectRoot parameter added
...

I made this change locally, and it fixed the issue I was experiencing on both my repo and the fresh reproducer repo, and on 2 different macs as well (Intel & M1 Pro).

However I am not at all familiar with react-native framework/tools development, so I am unable to verify that other stuff isn't impacted by this/run proper unit tests, etc. But... the code does appear to be isolated to this very specific case in the codegen dependency discovery process.

Other Environment Notes

  1. I tested using RN 0.74.2 and 0.75.2
  2. I tested on both Intel and M1 Pro mac books
  3. Using ZSH as my shell
  4. Using Kitty as my terminal
  5. nvm for node versioning
  6. rbenv for ruby versioning

Thanks for reading!

I'm even more in awe over React Native after having gone through this internal process step by step. Been using this stuff for over 7 years now, and wow have things evolved. Thanks to everyone involved. It's like magic!

Steps to reproduce

  1. Clone the reproducer
  2. Run yarn at the repo root
  3. cd packages/App2/ios
  4. pod install
  5. Observe the deprecation warning issued by codegen for the library @react-native-community/geolocation

NOTE This issue is NOT about that library. It just happens that it is useful to trigger the codegen deprecation warning that indicates dependency resolution is not working properly.

The deprecation warning is a reliable indicator that codegen has found the wrong version (the root node_modules vs the App2/node_modules) in this case.

3.2.1 should cause the warning 3.3.0 should not - App2 specifies 3.3.0 in its package.json

MORE NOTES

I initially tested with RN 74.2. The reproducer was init'ed fresh as a minimal demonstration using RN latest (75.2) and exhibits the same issue/fix behavior.

React Native Version

0.74.2, 0.75.2

Affected Platforms

Build - MacOS

Output of npx react-native info

info Fetching system and libraries information...
System:
  OS: macOS 14.6.1
  CPU: (10) arm64 Apple M1 Pro
  Memory: 554.67 MB / 16.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 20.16.0
    path: ~/.nvm/versions/node/v20.16.0/bin/node
  Yarn:
    version: 3.6.4
    path: ~/.nvm/versions/node/v20.16.0/bin/yarn
  npm:
    version: 10.8.1
    path: ~/.nvm/versions/node/v20.16.0/bin/npm
  Watchman:
    version: 2024.08.12.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.15.2
    path: /Users/michaeljarecki/.rbenv/shims/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.5
      - iOS 17.5
      - macOS 14.5
      - tvOS 17.5
      - visionOS 1.2
      - watchOS 10.5
  Android SDK: Not Found
IDEs:
  Android Studio: Not Found
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java: Not Found
  Ruby:
    version: 3.3.4
    path: /Users/michaeljarecki/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react: Not Found
  react-native: Not Found
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: true
  newArchEnabled: false

Stacktrace or Logs

NOTE: I replaced the name of the dep below to avoid any confusion that this issue is related to the external lib vs codegen. The deprecation warning itself is appropriate for the code codegen is finding, however, codegen is finding the wrong code <-- (that's the issue). The warning is a symptom of the underlying problem in this case and what tipped me off to the issue.

[Codegen] Searching for codegen-enabled libraries in the project dependencies.
[Codegen] Found <somedep>
[Codegen] CodegenConfig Deprecated Setup for <somedep>.
    The configuration file still contains the codegen in the libraries array.
    If possible, replace it with a single object.

BEFORE:
    {
      // ...
      "codegenConfig": {
        "libraries": [
          {
            "name": "libName1",
            "type": "all|components|modules",
            "jsSrcsRoot": "libName1/js"
          },
          {
            "name": "libName2",
            "type": "all|components|modules",
            "jsSrcsRoot": "libName2/src"
          }
        ]
      }
    }

    AFTER:
    {
      "codegenConfig": {
        "name": "libraries",
        "type": "all",
        "jsSrcsRoot": "."
      }
    }


### Reproducer

https://github.com/superguineapig/rntest

### Screenshots and Videos

_No response_
react-native-bot commented 2 months ago
:warning: Newer Version of React Native is Available!
:information_source: You are on a supported minor version, but it looks like there's a newer patch available - 0.74.5. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.
react-native-bot commented 2 months ago
:warning: Newer Version of React Native is Available!
:information_source: You are on a supported minor version, but it looks like there's a newer patch available - undefined. Please upgrade to the highest patch for your minor or latest and verify if the issue persists (alternatively, create a new project and repro the issue in it). If it does not repro, please let us know so we can close out this issue. This helps us ensure we are looking at issues that still exist in the most recent releases.
superguineapig commented 2 months ago

Tested at latest 0.75.2. Issue persists.

superguineapig commented 2 months ago

Tagging @dmytrorykun for visibility. Looks like you've been around these parts before ;-)

(my apologies for the spam if not)

dmytrorykun commented 2 months ago

@superguineapig thank you for this great issue report! Could you please try this PR https://github.com/facebook/react-native/pull/46229 and see if it fixes the problem.

superguineapig commented 2 months ago

@dmytrorykun Thank you for looking at this! The changes in the PR do appear to fix the issue.

Just for reference, I added some console logging to the findExternalLibraries function, and it appears that the explicit addition of the {paths: [projectRoot]} configuration is appropriately overriding the default module resolution for each call to resolve. 

...
[Codegen] Searching for codegen-enabled libraries in the project dependencies.
// Default resolution; note would never find app-level deps
[DEBUG] module.paths:
[
  '/Users/X/rntest/node_modules/react-native/scripts/codegen/node_modules',
  '/Users/X/rntest/node_modules/react-native/scripts/node_modules',
  '/Users/X/rntest/node_modules/react-native/node_modules',
  '/Users/X/rntest/node_modules',
  '/Users/X/node_modules',
  '/Users/node_modules',
  '/node_modules'
]
// Found an app-relative dep!
[DEBUG] found path: '/Users/X/rntest/packages/App2/node_modules/@react-native-community/geolocation/package.json'
[Codegen] Found @react-native-community/geolocation
// And it can still resolve workspace-root deps
[DEBUG] found path: '/Users/X/rntest/node_modules/react/package.json'
[DEBUG] found path: '/Users/X/rntest/node_modules/react-native/package.json'
[Codegen] Found react-native
...
...

This appears to fix the issue as described. Thank you again for your attention.

dmytrorykun commented 2 months ago

@superguineapig 

[DEBUG] module.paths:
[
  '/Users/X/rntest/node_modules/react-native/scripts/codegen/node_modules',
  '/Users/X/rntest/node_modules/react-native/scripts/node_modules',
  '/Users/X/rntest/node_modules/react-native/node_modules',
  '/Users/X/rntest/node_modules',
  '/Users/X/node_modules',
  '/Users/node_modules',
  '/node_modules'
]

This explains everything. require.resolve was trying to resolve the dependency relative to the codegen root. Not finding anything, it was making its way up to the monorepo root. The correct behaviour is to start at the project workspace directory.