If the user sets a custom node_modules location, always use it
#4684
Conversation
The previous version used the user-defined `REACT_NATIVE_NODE_MODULES_DIR` as a fallback to read the React Native `package.json` but forgot to use it later on when looking for `ReactCommon`.
This way, if the env var is not set the returned value will be `nil`, not a crash.
| react_native_node_modules_dir = File.join(File.dirname(`cd "#{Pod::Config.instance.installation_root.to_s}" && node --print "require.resolve('react-native/package.json')"`), '..') | ||
| react_native_json = try_to_parse_react_native_package_json(react_native_node_modules_dir) | ||
| # If the user set a custom location for where to look for React Native, use that | ||
| custom_react_native_node_modules_dir = ENV['REACT_NATIVE_NODE_MODULES_DIR'] | ||
| if custom_react_native_node_modules_dir.nil? | ||
| react_native_node_modules_dir = File.join(File.dirname(`cd "#{Pod::Config.instance.installation_root.to_s}" && node --print "require.resolve('react-native/package.json')"`), '..') | ||
| else | ||
| unless File.exist? custom_react_native_node_modules_dir | ||
| raise "[RNReanimated] The given React Native lookup path #{custom_react_native_node_modules_dir} does not exist" | ||
| end | ||
|
|
||
| if react_native_json == nil | ||
| # user configuration, just in case | ||
| node_modules_dir = ENV["REACT_NATIVE_NODE_MODULES_DIR"] |
There was a problem hiding this comment.
My rational for using REACT_NATIVE_NODE_MODULES_DIR before trying to resolve the path ourselves is that if the users set a custom value, they likely expect it to be used.
| lib_instances_in_reanimated_node_modules_array = Array.new | ||
| reanimated_instances = lib_instances_in_react_native_node_modules_array.length() | ||
| if react_native_info[:react_native_node_modules_dir] != react_native_info[:reanimated_node_modules_dir] | ||
| if react_native_info[:react_native_node_modules_dir] != react_native_info[:reanimated_node_modules_dir] && ENV['REACT_NATIVE_NODE_MODULES_DIR'].nil? |
There was a problem hiding this comment.
This is what resulted in the error
[!] Invalid `RNReanimated.podspec` file: different prefix: "" and "/Users/gio/Developer/oss/cocoapods-test/Pods".
which we got in the case of no available node_modules and a valid REACT_NATIVE_NODE_MODULES_DIR.
Basically, the setup had begun using one prefix but here it "forgot" about it.
|
Hi @mokagio. I'm testing your PR and can't help but notice there are some mistakes in the tests you provided, e.g. test1 is: rm -rf Pods \
&& rm -rf build \
&& pod cache clean RNReanimated \
&& TEST_MAIN=false \
REACT_NATIVE_NODE_MODULES_DIR=$PWD/../react-native-reanimated/node_modules \
pod installAnd I believe it should be: rm -rf Pods \
&& rm -rf build \
&& pod cache clean RNReanimated \
&& TEST_MAIN=false \
- REACT_NATIVE_NODE_MODULES_DIR=$PWD/../react-native-reanimated/node_modules \
- pod install
+ && pod installI could fix those for you but I'm not sure I will understand your intent in those tests. Would you be so kind to correct them? |
|
Hey @tjzel 👋 Thank you for taking a look at the PR and for double checking the steps I suggested. The version provided in the PR description is intentional. Unless I'm mistaken, adding If it makes testing clearer, I can update the commands to explicitly set all the env before running. Eg, the first test would go from to I provided those tests as one liners because it seemed to me like it would make for a clearer success/failure behavior. But we could also rewrite them as step-by-step one. Keen to hear what you think. Thanks. |
|
Actually what I meant is when I pasted them into my terminal (iTerm2) they just wouldn't execute due to incorrect syntax (although now when I look into it it looks fine 🤔). I will try to run those tests tomorrow 😃 |
|
@mokagio I'm sorry to say that but after some greater insight into those tests they have errors/typos in them. In the first test you are supposed to be checking Without With: Same for when I use Please double check your tests and their outcomes, some little details included, e.g.: - rm -rf Pods \
- && rm -rf build \
- && pod cache clean RNReanimated \
- && TEST_MAIN=false \
- REACT_NATIVE_NODE_MODULES_DIR=$PWD/../react-native-reanimated/node_modules \
- pod install
+ rm -rf Pods \
+ && rm -rf build \
+ && pod cache clean RNReanimated \
+ && TEST_MAIN=false \
+ REACT_NATIVE_NODE_MODULES_DIR=$PWD/../react-native-reanimated/node_modules \
+ pod installbecause in this case the trailing space after |
tjzel
added
the
Close when stale
|
Thank you for submitting this pull request! I value your time and dedication! 🙌 However, we have not fully comprehended every aspect of these changes, and since there has been little activity on this issue, I have made the decision to close it. If you wish to continue working on this pull request, we are still open to collaboration. Have a grate day! 🤗 |
Summary
Users can provide a custom path for where the CocoaPods setup should look for the React Native
node_modules.The previous version used the user-defined
REACT_NATIVE_NODE_MODULES_DIRas a fallback to read the React Nativepackage.jsonbut forgot to use it later on when looking forReactCommon.Test plan
Admittedly, the main issue this PR aims to solve occurs only in a contrived setup where
node --print "require.resolve('react-native/package.json')"fails.Such a setup can be reproduced by
cocoapods-testin the same parent folder as this repositoryPodfilewith the following content:Before starting testing, we can set the path to React Native for the
Podfileto use (different from the one that the library uses), e.g.:We can now test the following scenarios
1.
main+ noREACT_NATIVE_NODE_MODULES_DIR– FailsNote: The
rm -rfs andpod cache clean...commands make sure we test from a fresh setup.2.
main+REACT_NATIVE_NODE_MODULES_DIR– Fails with different messageNotice the uses of
$PWD, I run into issues when using a relative path. I haven't had a chance to investigate them yet. I think this PR consists in an improvement regardless.3.
main+ InvalidREACT_NATIVE_NODE_MODULES_DIR– Fails with different messageThis time, at least, the message points to using
REACT_NATIVE_NODE_MODULES_DIRunlike the previous ones—even though because of the issues already highlighted, it won't work4. This PR + no
REACT_NATIVE_NODE_MODULES_DIR– Fails (as expected) with better messageStill fails, as expected when there are no available
node_modulesbut at least it suggests using theREACT_NATIVE_NODE_MODULES_DIRenvironment variable.5.
main+REACT_NATIVE_NODE_MODULES_DIR– WorksThis one works, which is the desired behavior 😄
6.
main+ InvalidREACT_NATIVE_NODE_MODULES_DIR– Fails (as expected) with better messageThe failure is expected because the
node_modulespath is invalid. But in this version a user will know the path the install process used, which will be helpful in fixing the issue.