[0.73] `react-native.config.js` is displayed in the upgrade helper

rickhanlonii commented 9 months ago

Description

I was upgrading my app from 72 to 73 and reviewing the Upgrade Helper changes, and I saw a new file react-native.config.js:

I looked in the react-native repo, but this file isn't in the template. So I looked in the CLI and found the change that includes this comment:

/*
Starting from 0.73, react-native.config.js is created by CLI during the init process.
It contains automaticPodsInstallation flag set to true by default.
This flag is used by CLI to determine whether to install CocoaPods dependencies when running ios commands or not.
It's created by CLI rather than being a part of a template to avoid displaying this file in the Upgrade Helper,
as it might bring confusion for existing projects where this change might not be applicable.
For more details, see https://github.com/react-native-community/cli/blob/main/docs/projects.md#projectiosautomaticpodsinstallation
*/

So the intention was to not check-in the file to the template, so the helper didn't include it. But for some reason, the helper does include it.

rickhanlonii commented 9 months ago

Personally, I feel like when I run the init command, I should be able to see all of the same files created in the react-native template repo. And since this is already there, I would probably just check it in.

rickhanlonii commented 9 months ago

The default value of this option is true, right? Why create the file at all then? Why not include instructions for setting it to false if you don't want the behavior, no file needed?

TMisiukiewicz commented 9 months ago

@rickhanlonii indeed it shouldn't be displayed in the upgrade helper. Initially our idea was to support automatic pods installation without any additional setup, however we decided to hide this feature behind this flag and enable it by default only for freshly started projects. For all the projects upgrading to newest version this should be optional, as there might be various reasons where this workflow would not fit:

the template is using RubyGems to install CocoaPods (and CLI covers that scenario), but e.g. brownfield setups might be slightly different and use e.g Homebrew
CocoaPods might be installed by RubyGems, but on the user global level
pod install is expensive and might take a lot of time on complex projects. Those kind of projects very often have Pods directory checked into source control so pod install is not desired

For those reasons, we wanted to run pod install when using ios-specific commands by default only for projects started with init from version 0.73. It runs automatically if pods are not installed, or there is a new native dependency discovered in config, so user don't have to remember about installing pods manually. A warning that it might not work properly when upgrading RN is in the docs. This workflow also speeds up the init process, where pods installation is optional now.

I already checked Upgrade Helper but couldn't find anything that prevents from showing specific files. Maybe it can be done in rn-diff-purge? Would love to get in touch with someone maintaining it to find the proper solution. An alternative solution is adding a comment in the file itself, indicating that it is created by the CLI and it's optional when upgrading to newer RN version, however I'd prefer that file to not be displayed at all.

TMisiukiewicz commented 9 months ago

One more solution that comes to my mind is creating this file only if process.env.CI is false, I assume diff-purge is running a job to generate a fresh project for each version. I assume init is not a command that is ran on CIs heavily, it should not break anything for those who do that.

cortinico commented 9 months ago

I second @rickhanlonii thoughts as we should strive to don't manipulate the template in the init command.

@rickhanlonii indeed it shouldn't be displayed in the upgrade helper. Initially our idea was to support automatic pods installation without any additional setup, however we decided to hide this feature behind this flag and enable it by default only for freshly started projects. For all the projects upgrading to newest version this should be optional, as there might be various reasons where this workflow would not fit:

Can we do the following changes instead:

Assume automaticPodsInstallation == true for everyone in 0.73 (both newly created and upgrade projects)
Remove the react-native.config.js file generation in the init command
Update the release blogpost to mention that this is a behavior change, and explain how to opt-out
(Optional) show a message on the first build-ios/run-ios that is triggering pod install by saying:

Starting with React Native 0.73, we will now pod install automatically if you attempt to build-ios/run-ios and you haven't run pod install before. You can opt-out from this behavior by setting automaticPodsInstallation = false in your react-native.config.js file as described here

TMisiukiewicz commented 9 months ago

@tido64 what are your thoughts about it? I remember we discussed that when it was initially introduced

tido64 commented 9 months ago

@tido64 what are your thoughts about it? I remember we discussed that when it was initially introduced

The current implementation is flawed and will break all existing setups internally at Microsoft. There is no way you can ship this as opt-out in its current shape. For transparency, here's the list of issues I posted:

It assumes how and where CocoaPods is installed
- It calls bundle exec pod install, which assumes that users installed CocoaPods via RubyGems. This may not always be the case — you can also install it with Homebrew or MacPorts. CI services may be even more creative.
- Even if CocoaPods was installed via RubyGems, it could've been done on the user global level. Meaning that it will fail if the current Gemfile does not include CocoaPods. Ruby will throw an exception if that's the case, failing run-ios. This is the original issue I reported in the 0.73 roadmap discussion.
The current implementation does not reuse the hashes in Podfile.lock and Pods/Manifest.lock. This means that it won't pick up changes that are done outside package.json. For instance, if someone updated yarn.lock only.
pod install is prohibitively expensive on larger projects. We have apps where this can take 30 minutes or more. It's so slow, they check in the Pods folder so that devs don't have to run it unless they're updating dependencies. With this version, if you pull down the latest changes and there have been dependency updates, CLI will unnecessarily run pod install. This is a big no for us.
Checking in Pods folder is common practice: https://guides.cocoapods.org/using/using-cocoapods.html#should-i-check-the-pods-directory-into-source-control

tido64 commented 9 months ago

cc @kelset @SaadNajmi

cortinico commented 9 months ago

The current implementation is flawed and will break all existing setups internally at Microsoft. There is no way you can ship this as opt-out in its current shape. For transparency, here's the list of issues I posted:

@tido64 do you have suggestions on how to make it better?

tido64 commented 9 months ago

@tido64 do you have suggestions on how to make it better?

Besides fixing all the issues? How about not making run-ios do so many things? Personally, I would prefer telling people to run doctor instead. You can add as many fixers as you want there instead.

Edit: As for immediate actions: Make it opt in. Make sure its flaws are documented.

szymonrybczak commented 8 months ago

So, we removed creating react-native.config.js file from init command in https://github.com/react-native-community/cli/pull/2203. Also we re-generated diff for Upgrade Helper, so that when in 0.73.0 and 0.73.1 react-native.config.js is no more visible 👍

We're now asking user if they want to install pods during init, added in: https://github.com/react-native-community/cli/pull/2204

react-native-community / cli

[0.73] `react-native.config.js` is displayed in the upgrade helper #2202

Description