fix(iOS): pod install fails outside ios folder

[tido64]

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]

cc @alloy, @grabbou, @kelset

[kelset]

LGTM 👍

[alloy]

Nice 👌

[grabbou]

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])

[grabbou]

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.

[grabbou]

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)

[grabbou]

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.

[tido64]

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.

[tido64]

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]

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]

@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]

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]

@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]

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]

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]

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]

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

[pull-bot]

	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]

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]

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]

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]

Superseded by #30449.