fix(iOS): `pod install` fails outside `ios` folder #30469

tido64 · 2020-11-24T16:13:27Z

Summary

Invoking pod install outside the ios folder fails because codegen_pre_install() looks for node_modules in the parent folder.

example % pod install --project-directory=ios
Auto-linking React Native module for target `ReactTestApp`: ReactTestApp-DevSupport
Analyzing dependencies
Downloading dependencies
internal/modules/cjs/loader.js:834
  throw err;
  ^

Error: Cannot find module '/~/node_modules/react-native-codegen/lib/cli/combine/combine-js-to-schema-cli.js'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:831:15)
    at Function.Module._load (internal/modules/cjs/loader.js:687:27)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:60:12)
    at internal/main/run_main_module.js:17:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}
[!] An error occurred while processing the pre-install hook of the Podfile.

Could not generate Native Module schema

Changelog

[iOS] [Fixed] - pod install --project-directory=ios fails to find react-native-codegen

Test Plan

Both pod install (inside ios folder) and pod install --project-directory=ios (outside ios folder) should work.

tido64 · 2020-11-24T16:13:52Z

cc @alloy, @grabbou, @kelset

kelset · 2020-11-24T16:16:48Z

LGTM 👍

scripts/react_native_pods.rb

alloy

Nice 👌

grabbou · 2020-11-25T12:48:24Z

scripts/react_native_pods.rb

+  # Path to React Native relative to where `pod install` was invoked. This can
+  # sometimes be inside the `ios` folder (i.e. `pod install `), or outside
+  # (e.g. `pod install --project_directory=ios`).
+  prefix = options[:path] ||= resolve_module "react-native"


To keep it the same with use_react_native, we should probably just stick to options[:path] and pass it like here:

react-native/template/ios/Podfile

Lines 7 to 9 in 355239b

config = use_native_modules!

use_react_native!(:path => config[:reactNativePath])

reactNativePath can be programatically set in the react-native.config.js and might be not what this resolves to in use_react_native!.

Example: https://github.com/facebook/react-native/blob/master/react-native.config.js#L32

Set to . for React Native root, this will not work inside React Native repository as there's no module to require.resolve to.

This can lead to some unexpected issues.

Looks like codegen_pre_install(:path => config[:reactNativePath]) present in a default template would resolve all issues without any further code changes.

The reason is: when we tell codegen_pre_install to use config[:reactNativePath] as I said above, which is an absolute path to React Native location, then, path to codegen would always be correct, since it's going to be absolute too.

("#{prefix}/../react-native-codegen" out of prefix which is absolute)

Technically, the assumption of react-native-codegen living side by side react-native can be false in the following scenario:

node_modules/react-native-codegen (0.0.7) packageA/node_modules/react-native (0.64.0-rc.0) packageB/node_modules/react-native (0.64.0-rc.1)

In other words, two React Native packages sharing same React Native codegen version.

In that case, it may be worth to leave resolve_module just for react-native-codegen or... we make sure that react-native-codegen is always versioned together with the React Native (same version as the RC, with patch bump).

This is the idea going forward, but at the time of writing, the aforementioned scenario is technically possible.

Not quite sure I understand what you mean here… The root of this issue is that we cannot use the same path for both use_react_native! and codegen_pre_install. use_react_native! needs paths that are relative to the Podfile because CocoaPods will be resolving modules later. codegen_pre_install, on the other hand, needs the paths to be relative to the current working directory because it is invoked right away.

Sorry, didn't see your replies until after I had posted this. The scenario you mentioned in the last post is a very real one for us. I'd rather leave it like this.

grabbou

Requested changes in a comment - I believe we should stick to reactNativePath like in use_react_native! to keep it synchronised. Then, we don't need to resolve anything.

grabbou · 2020-11-25T12:59:05Z

I am also trying to understand why did we introduce another script, codegen_pre_install, at all? It seems to be required and needs to be always included. Otherwise iOS apps will fail to build as there are missing files.

Why don't we just make it part of use_react_native? Is it because of pre_install phase?

https://github.com/facebook/react-native/blob/master/template/ios/Podfile#L16

CC: @alloy

alloy · 2020-11-25T13:57:53Z

@grabbou It’s a good question, but think that's moot at this point, as the intent is to move invocation of the codegen compiler out of the Podfile entirely as it needs to be part of the build pipeline: #30449

I.e. I don’t think there needs to be too much work done here, as it will likely be removed soon anyways.

tido64 · 2020-11-25T14:10:41Z

I.e. I don’t think there needs to be too much work done here, as it will likely be removed soon anyways.

@alloy: If codegen is going to be moved out of Podfile, wouldn't it make sense to do that now before we ship 0.64? Are we telling consumers to add this function, then remove it in the next version?

grabbou · 2020-11-25T14:26:35Z

@tido64, the PR referenced by @alloy is listed on the release thread as a "non RC blocking" to be resolved ahead of stable release. Waiting for @hramos to be back next week to get better understanding of the path going forward for codegen and whether this one is indeed required for stable.

grabbou · 2020-11-25T14:27:04Z

It makes sense to update the Podfile with a path tho, as a part of making it easier for users to play around with the RC.

kelset · 2020-11-25T15:02:47Z

It makes sense to update the Podfile with a path tho, as a part of making it easier for users to play around with the RC.

Maybe then we should have this PR point directly to the 0.64 branch instead on master? That way we can roll it back once we have the proper codegen fix?

alloy · 2020-11-25T15:25:10Z

Maybe then we should have this PR point directly to the 0.64 branch instead on master? That way we can roll it back once we have the proper codegen fix?

Yeah I think this makes sense 👍

tido64 · 2020-11-25T15:28:05Z

Note to self: Don't change the base branch on GitHub before rebasing the branch.

pull-bot · 2020-11-25T15:35:55Z

	Warnings
⚠️	❔ Base Branch - The base branch for this PR is something other than `master`. Are you sure you want to merge these changes into a stable release? If you are interested in backporting updates to an older release, the suggested approach is to land those changes on `master` first and then cherry-pick the commits into the branch for that release. The Releases Guide has more information.

Generated by 🚫 dangerJS against 8d5ef5b

alloy

The remaining CI failures appear to be because of the missing shelljs dependency that @grabbou should already have a PR for.

grabbou · 2020-12-01T16:01:03Z

I am happy with partially accepting it as it will go away anyway. But... as I mentioned in my comment here, I would prefer we do not resolve_module "react-native", but use :reactNativePath, just like we do for use_react_native, to keep it aligned.

tido64 · 2020-12-02T07:43:08Z

I am happy with partially accepting it as it will go away anyway. But... as I mentioned in my comment here, I would prefer we do not resolve_module "react-native", but use :reactNativePath, just like we do for use_react_native, to keep it aligned.

I don't mind either way as this will be move out of Podfile but as you also mentioned in #30469 (comment), hoisting is an issue and it does affect us. Using the same :reactNativePath as use_react_native doesn't make much sense to me when they are configuring commands that are run at different times and from different paths. I'd rather not make any more changes to this PR until the real solution comes along.

alloy · 2020-12-02T13:47:54Z

It looks like #30449 will be in the next RC, but if that doesn’t end up happening I think we can pull in this change for the time being. I don’t feel knowledgeable enough about the monorepo aspect of path handling and wether or not that should hold this up for that short period.

tido64 · 2020-12-19T10:51:02Z

Superseded by #30449.

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner labels Nov 24, 2020

tido64 mentioned this pull request Nov 24, 2020

Road to v0.64 Discussion react-native-community/releases#214

Closed

36 tasks

react-native-bot added Bug Platform: iOS iOS applications. labels Nov 24, 2020

Titozzz reviewed Nov 24, 2020

View reviewed changes

scripts/react_native_pods.rb Show resolved Hide resolved

alloy approved these changes Nov 25, 2020

View reviewed changes

grabbou reviewed Nov 25, 2020

View reviewed changes

grabbou suggested changes Nov 25, 2020

View reviewed changes

tido64 changed the base branch from master to 0.64-stable November 25, 2020 15:26

tido64 requested review from cpojer, hramos, mhorowitz and shergin as code owners November 25, 2020 15:26

fix(iOS): pod install fails outside ios folder

8d5ef5b

tido64 force-pushed the fix-codegen_pre_install branch from 355239b to 8d5ef5b Compare November 25, 2020 15:29

alloy approved these changes Nov 30, 2020

View reviewed changes

tido64 closed this Dec 19, 2020

	config = use_native_modules!

	use_react_native!(:path => config[:reactNativePath])

fix(iOS): pod install fails outside ios folder #30469

fix(iOS): pod install fails outside ios folder #30469

Uh oh!

Conversation

tido64 commented Nov 24, 2020

Summary

Changelog

Test Plan

Uh oh!

tido64 commented Nov 24, 2020

Uh oh!

kelset commented Nov 24, 2020

Uh oh!

Uh oh!

alloy left a comment

Choose a reason for hiding this comment

Uh oh!

grabbou Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

grabbou Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grabbou Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grabbou Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

tido64 Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tido64 Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

grabbou left a comment

Choose a reason for hiding this comment

Uh oh!

grabbou commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alloy commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tido64 commented Nov 25, 2020

Uh oh!

grabbou commented Nov 25, 2020

Uh oh!

grabbou commented Nov 25, 2020

Uh oh!

kelset commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alloy commented Nov 25, 2020

Uh oh!

tido64 commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pull-bot commented Nov 25, 2020

Uh oh!

alloy left a comment

Choose a reason for hiding this comment

Uh oh!

grabbou commented Dec 1, 2020

Uh oh!

tido64 commented Dec 2, 2020

Uh oh!

alloy commented Dec 2, 2020

Uh oh!

tido64 commented Dec 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

fix(iOS): `pod install` fails outside `ios` folder #30469

fix(iOS): `pod install` fails outside `ios` folder #30469

grabbou Nov 25, 2020 •

edited

Loading

grabbou Nov 25, 2020 •

edited

Loading

tido64 Nov 25, 2020 •

edited

Loading

grabbou commented Nov 25, 2020 •

edited

Loading

alloy commented Nov 25, 2020 •

edited

Loading

kelset commented Nov 25, 2020 •

edited

Loading

tido64 commented Nov 25, 2020 •

edited

Loading