Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
rickhanlonii opened this issue Dec 11, 2023 · 11 comments
Closed

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

rickhanlonii opened this issue Dec 11, 2023 · 11 comments

Comments

@rickhanlonii
Copy link
Collaborator

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:

Screenshot 2023-12-11 at 5 48 43 PM

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Collaborator

@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
Copy link
Collaborator

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
Copy link
Member

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:

  1. Assume automaticPodsInstallation == true for everyone in 0.73 (both newly created and upgrade projects)
  2. Remove the react-native.config.js file generation in the init command
  3. Update the release blogpost to mention that this is a behavior change, and explain how to opt-out
  4. (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
Copy link
Collaborator

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

@tido64
Copy link
Contributor

tido64 commented Dec 12, 2023

@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
Copy link
Contributor

tido64 commented Dec 12, 2023

cc @kelset @Saadnajmi

@cortinico
Copy link
Member

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
Copy link
Contributor

tido64 commented Dec 12, 2023

@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
Copy link
Collaborator

So, we removed creating react-native.config.js file from init command in #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: #2204

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants