Fix use_react_native to support absolute paths #37545

gabrieldonadel · 2023-05-24T03:10:19Z

Summary:

While setting up a monorepo that required a custom react-native path location (react-native-macos in my case) I was getting the following error when running pod install

That's because build_codegen and checkAndGenerateEmptyThirdPartyProvider functions don't check if the given react_native_path is absolute or relative.

This PR fixes this problem by checking if react_native_path starts with /

Changelog:

[IOS] [FIXED] - Fix use_react_native to support custom react native absolute paths

Test Plan:

Modify rn-tester/Podfile to use an absolute path when calling use_react_native

E.g.

rn_path = File.dirname(`node --print "require.resolve('react-native/package.json')"`) 

use_react_native!(
  path: rn_path, 
  ...
)

then run pod install

analysis-bot · 2023-05-24T03:37:02Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,739,360	-5,061
android	hermes	armeabi-v7a	8,050,513	-6,114
android	hermes	x86	9,229,736	-5,812
android	hermes	x86_64	9,081,581	-4,797
android	jsc	arm64-v8a	9,302,721	-4,544
android	jsc	armeabi-v7a	8,491,196	-5,611
android	jsc	x86	9,363,975	-5,306
android	jsc	x86_64	9,619,912	-4,292

Base commit: 4540668
Branch: main

cipolleschi

Hi @gabrieldonadel! Thank you so much for the contribution.

I added a suggestion to improve readability and maintainability.
Also, there should be some tests in __tests__/codegen-test.rb: it would be great to duplicate one of those tests with an absolute path, so we make sure not to break this in the future!

Thank you so much.

packages/react-native/scripts/cocoapods/codegen.rb

cipolleschi

Hi @gabrieldonadel! Thank you so much for the contribution.

I added a suggestion to improve readability and maintainability.
Also, there should be some tests in __tests__/codegen-test.rb: it would be great to duplicate one of those tests with an absolute path, so we make sure not to break this in the future!

Thank you so much.

gabrieldonadel · 2023-05-24T12:26:22Z

Hi @cipolleschi, thanks for the feedback, I've just pushed a commit updating the code to use the basePath function along with a new test case on codegen-test.rb ensuring that absolute paths work

gabrieldonadel · 2023-05-26T11:13:25Z

Hi @cipolleschi, is there something else that needs to be done here?

cipolleschi · 2023-05-29T14:00:13Z

No, sorry... I've been caught up in the release of RC.4 and had little time to followup with the PRs. Importing it now.

facebook-github-bot · 2023-05-30T14:22:01Z

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

NickGerleman · 2023-05-30T17:42:12Z

Could we use something like File.expand_path instead of rolling this ourselves?

cipolleschi · 2023-05-31T09:10:30Z

packages/react-native/scripts/cocoapods/codegen.rb

@@ -108,3 +108,11 @@ def run_codegen!(
    codegen_utils.generate_react_codegen_podspec!(react_codegen_spec, codegen_output_dir)
  end
 end
+
+def basePath(react_native_path, relative_installation_root)
+  if react_native_path.start_with?("/")


Suggested change

if react_native_path.start_with?("/")

if Pathname.new(react_native_path).absolute

This suggestion also need a require 'pathname' after the license.
(See here for reference)

cipolleschi · 2023-05-31T09:10:32Z

packages/react-native/scripts/cocoapods/codegen.rb

@@ -11,8 +11,8 @@
 # - dir_manager: a class that implements the `Dir` interface. Defaults to `Dir`, the Dependency can be injected for testing purposes.
 # @throws an error if it could not find the codegen folder.
 def build_codegen!(react_native_path, relative_installation_root, dir_manager: Dir)
-    codegen_repo_path = "#{relative_installation_root}/#{react_native_path}/../react-native-codegen";
-    codegen_npm_path = "#{relative_installation_root}/#{react_native_path}/../@react-native/codegen";
+    codegen_repo_path = "#{basePath(react_native_path, relative_installation_root)}/../react-native-codegen";


Following up with @NickGerleman suggestion, probably it is better to use some system APIs...

Suggested change

codegen_repo_path = "#{basePath(react_native_path, relative_installation_root)}/../react-native-codegen";

codegen_repo_path = File.join(basePath(react_native_path, relative_installation_root), "..", "react-native-codegen")

Changing this seems to break some codegen tests, I'll try to investigate what's going on latter today

cipolleschi · 2023-05-31T09:10:56Z

packages/react-native/scripts/cocoapods/codegen.rb

+  if react_native_path.start_with?("/")
+    react_native_path
+  else
+    "#{relative_installation_root.to_s}/#{react_native_path}"


Suggested change

"#{relative_installation_root.to_s}/#{react_native_path}"

File.join(relative_installation_root.to_s, react_native_path)"

cipolleschi

Following @NickGerleman right suggestion, I left some comments to improve the codebase! :D

Thank you both for the review and the effort!

cipolleschi · 2023-06-14T15:45:58Z

/rebase

gabrieldonadel · 2023-06-16T15:52:31Z

Hi @cipolleschi, sorry for the delay on this, it seems that these changes to use system APIs kinda break all tests involving paths, what should we do about it? Should I update the tests or should we revert to using the original implementation? I'm afraid that using system APIs could break something else as it affected a bunch of tests

cipolleschi · 2023-06-19T15:15:02Z

So... the problem with these tests is that they are using a Mock for Pathname that can be found here: https://github.com/facebook/react-native/blob/main/packages/react-native/scripts/cocoapods/__tests__/test_utils/PathnameMock.rb

So, we have two options:

enhance the PathnameMock to support absolute
we revert to the original version

In theory, using system API should be the right way to go.
I'd say that we can timebox that to 1 hr, for example, to try and fix the tests. If we can't let's revert and I'll try that out myself once it landed. How do you feel about this?

facebook-github-bot · 2023-06-30T11:18:32Z

@cipolleschi merged this pull request in 835f62c.

gabrieldonadel · 2023-06-30T11:55:40Z

Oops, sorry for taking so long I this, I had this on my todo list for a while but couldn't get to work on that yet 😅

cipolleschi · 2023-06-30T15:37:17Z

no problem! with the approaching of the end of the half I ended up with some free time on my hands and I was clearing up my queue of open things. It took actually very little effort! ;)

Anyway, thank you for carrying on most of the work! :D

Fix use_react_native to support absolute paths

6b58c75

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: Expo Partner: Expo Partner labels May 24, 2023

cipolleschi requested changes May 24, 2023

View reviewed changes

packages/react-native/scripts/cocoapods/codegen.rb Outdated Show resolved Hide resolved

cipolleschi requested changes May 24, 2023

View reviewed changes

gabrieldonadel added 2 commits May 24, 2023 09:09

Extract absolute path check into a function

ae3c14e

Update codegen tests

c243149

gabrieldonadel requested a review from cipolleschi May 24, 2023 12:26

Fix rn test path for CI

4ffdb48

gabrieldonadel force-pushed the @gabrieldonadel/fix-codegen-rb-scripts branch from 3dffd62 to 4ffdb48 Compare May 24, 2023 13:49

cipolleschi reviewed May 31, 2023

View reviewed changes

cipolleschi requested changes May 31, 2023

View reviewed changes

Use system APIs

7594dde

gabrieldonadel force-pushed the @gabrieldonadel/fix-codegen-rb-scripts branch from 4099265 to 7594dde Compare June 7, 2023 21:30

facebook-github-bot closed this in 835f62c Jun 30, 2023

facebook-github-bot added the Merged This PR has been merged. label Jun 30, 2023

gabrieldonadel deleted the @gabrieldonadel/fix-codegen-rb-scripts branch June 30, 2023 11:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix use_react_native to support absolute paths #37545

Fix use_react_native to support absolute paths #37545

gabrieldonadel commented May 24, 2023

analysis-bot commented May 24, 2023 •

edited

Loading

cipolleschi left a comment

cipolleschi left a comment

gabrieldonadel commented May 24, 2023

gabrieldonadel commented May 26, 2023

cipolleschi commented May 29, 2023

facebook-github-bot commented May 30, 2023

NickGerleman commented May 30, 2023

cipolleschi May 31, 2023

cipolleschi May 31, 2023

gabrieldonadel May 31, 2023

cipolleschi May 31, 2023

cipolleschi left a comment

cipolleschi commented Jun 14, 2023

gabrieldonadel commented Jun 16, 2023

cipolleschi commented Jun 19, 2023

facebook-github-bot commented Jun 30, 2023

gabrieldonadel commented Jun 30, 2023

cipolleschi commented Jun 30, 2023 •

edited

Loading

	if react_native_path.start_with?("/")
	if Pathname.new(react_native_path).absolute

	codegen_repo_path = "#{basePath(react_native_path, relative_installation_root)}/../react-native-codegen";
	codegen_repo_path = File.join(basePath(react_native_path, relative_installation_root), "..", "react-native-codegen")

	"#{relative_installation_root.to_s}/#{react_native_path}"
	File.join(relative_installation_root.to_s, react_native_path)"

Fix use_react_native to support absolute paths #37545

Fix use_react_native to support absolute paths #37545

Conversation

gabrieldonadel commented May 24, 2023

Summary:

Changelog:

Test Plan:

analysis-bot commented May 24, 2023 • edited Loading

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi left a comment

Choose a reason for hiding this comment

gabrieldonadel commented May 24, 2023

gabrieldonadel commented May 26, 2023

cipolleschi commented May 29, 2023

facebook-github-bot commented May 30, 2023

NickGerleman commented May 30, 2023

cipolleschi May 31, 2023

Choose a reason for hiding this comment

cipolleschi May 31, 2023

Choose a reason for hiding this comment

gabrieldonadel May 31, 2023

Choose a reason for hiding this comment

cipolleschi May 31, 2023

Choose a reason for hiding this comment

cipolleschi left a comment

Choose a reason for hiding this comment

cipolleschi commented Jun 14, 2023

gabrieldonadel commented Jun 16, 2023

cipolleschi commented Jun 19, 2023

facebook-github-bot commented Jun 30, 2023

gabrieldonadel commented Jun 30, 2023

cipolleschi commented Jun 30, 2023 • edited Loading

analysis-bot commented May 24, 2023 •

edited

Loading

cipolleschi commented Jun 30, 2023 •

edited

Loading