fix: fix @react-native-community/cli not being found in monorepos

[tido64]

Summary:

Fix @react-native-community/cli not being found in monorepos even though it is installed.

Note that we are making the assumption that process.cwd() returns the project root. This is the same assumption that @react-native-community/cli makes. Specifically, findProjectRoot() has an optional argument that defaults to process.cwd():

• findProjectRoot()
• Which gets called without arguments in loadConfig()
• loadConfig() gets called from setupAndRun(), also without project root set

As far as I can see, the project root argument is only ever used in tests.

Changelog:

[GENERAL] [FIXED] - Fix @react-native-community/cli not being found in monorepos

Test Plan:

1. Clone/check out this branch: chore: bump react-native to 0.76 microsoft/rnx-kit#3409
2. Run yarn react-native config

Before:

⚠ react-native depends on @react-native-community/cli for cli commands. To fix update your package.json to include:
"devDependencies": {
"@react-native-community/cli": "latest",
}

After: The usual output of react-native config

[facebook-github-bot]

@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[robhogan]

What is process.cwd() in this context? It seems fragile to assume that's where the dependency is declared.

[tido64]

It's where the react-native command is being executed from, which should be project root. I am not familiar with use cases where it's being executed elsewhere.

[blakef]

@tido64 how do I reproduce this issue locally? I'm trying to understand what's going on with pnpm.

[tido64]

@tido64 how do I reproduce this issue locally? I'm trying to understand what's going on with pnpm.

Follow the repro steps in the description.

[blakef]

@tido64 thanks for working with me on this one. I wonder if we can tweak this approach slightly. The way I could reproduce this locally was with Yarn Berry using the pnpm nodeLinker.

As a more general solution, we can look at updating the module.paths global:

functionyarnPnpmWorkaround(){constproject=process.cwd();if(module.paths.indexOf(project)===-1){module.paths.push(project)}}

This way we get the default module lookup paths, but add the workaround for the issue you're seeing in monorepos with pnpm linking. We could do more to only apply this when needed?

constisYarnBerryPnpm=()=>/yarn\/[^1]/.test(process.env.npm_config_user_data)&&execSync('yarn config get nodeLinker').toString().trim()==='pnpm');constisPnpm=()=>/pnpm\//.test(process.env.npm_config_user_data)constmustUpdatePaths=isPnpm()||isYarnBerryPnpm();functionupdatePaths(){if(!mustUpdatePaths)return;yarnPnpmWorkaround();}

@robhogan thoughts?

[tido64]

As a more general solution, we can look at updating the module.paths global:

functionyarnPnpmWorkaround(){constproject=process.cwd();if(module.paths.indexOf(project)===-1){module.paths.push(project)}}

This way we get the default module lookup paths, but add the workaround for the issue you're seeing in monorepos with pnpm linking. We could do more to only apply this when needed?

I think this is a big no-no. This changes the paths for everything. There are too many side-effects that can occur.

Monorepos with a pnpm setup can reproduce this fairly consistently, but even with "normal" node_modules layout, there are no guarantees for how a package is hoisted. If there are multiple versions of CLI, but only one of RN, Yarn can (and probably will) hoist only RN to repo root, while leaving CLI in the individual packages. You'll run into the same issues there.

[robhogan]

IMO the original approach (assuming project.cwd() is the root) is ok as a heuristic, but potentially breaks workflows that execute cli.js directly, eg from native build scripts, and I'm honestly not sure how it would behave when using a package manager with a --cwd or --workspace argument to execute from outside the project root - probably ok but it'd need checking with all the major ones.

If we wanted to pick this into 0.76 it'd have to be non-breaking, and the safest way to do that is to fall back to a cwd-based resolution only if the assume-hoisted resolution fails (eg, paths: [__dirname, process.cwd()]). That's still best-effort and not fully correct, but it's strictly better than what we have, whereas the PR as is trades one unsafe assumption for another.

(The blessed way to do this is theoretically with optional peer dependencies, but I don't know what the state of support is amongst package managers these days, and afaik react-native/cli.js is going away soon anyway?)

[tido64]

If we wanted to pick this into 0.76 it'd have to be non-breaking, and the safest way to do that is to fall back to a cwd-based resolution only if the assume-hoisted resolution fails (eg, paths: [__dirname, process.cwd()]). That's still best-effort and not fully correct, but it's strictly better than what we have, whereas the PR as is trades one unsafe assumption for another.

Just so I don't misunderstand, you want this change in main? Also, would it make more sense to reverse the check? Do cwd based first, then assume-hoisted? Even if CLI was hoisted, we have no idea whether the right version was hoisted. Wouldn't it be safer to try cwd first, then hoisted?

[blakef]

Just so I don't misunderstand, you want this change in main? Also, would it make more sense to reverse the check? Do cwd based first, then assume-hoisted? Even if CLI was hoisted, we have no idea whether the right version was hoisted. Wouldn't it be safer to try cwd first, then hoisted?

I think it makes sense to go in main and we can cherry-pick for 0.76.2.

My thinking is to put cwd last since:

1. non-pnpm: should lookup using the regular order of paths (so we're not breaking how things work today for folks) and then cwd.
2. pnpm: these lookup path should miss always miss as things are, and cwd is the only thing that should hit.

Does that seem reasonable?

[tido64]

My reason for going with cwd first is to avoid the scenario where we have multiple versions of React Native in a monorepo and the layout looks something like this:

workspace
├── node_modules
│ ├── @react-native-community/cli (15.0, for 0.76)
│ └── react-native (0.77)
└── packages
├── new-app
│ └── node_modules
│ └── @react-native-community/cli (16.0, for 0.77)
└── old-app
└── node_modules
└── react-native (0.76)

We've hit similar scenarios before (albeit with Metro).

[blakef]

Cool, that makes sense. Update to that please.

Update: Discussed with @robhogan some more. If you update to the equivalent of this:

try{returnrequire('@react-native-community/cli');}catch{returnrequire(require.resolve('@react-native-community/cli',{paths: [startDir]}));}

and target 0.76-stable, that unblocks you for that release and gives us more time to consider things for 0.77.

[tido64]

@robhogan Could you also have a look at this again? Thanks ❤️

[facebook-github-bot]

@robhogan merged this pull request in a672a4d.

[react-native-bot]

This pull request was successfully merged by @tido64 in a672a4d

^{When will my fix make it into a release? | How to file a pick request?}

[react-native-bot]

This pull request was successfully merged by @tido64 in 9ba96ad

^{When will my fix make it into a release? | How to file a pick request?}