Move `use_flipper` logic inside `use_react_native` and simplify the Flipper dependencies logic #33882

f-meloni · 2022-05-20T14:34:22Z

Summary

This PR tries to simplify the use_flipper logic:

makes use_flipper a configuration inside use_react_native's options
uses the already present production flag in the use_react_native's options to decide if add or not the Flipper pods
Simplifies the logic to download the flipper dependencies

This PR also adds a workaround for #33764

Changelog

[iOS] [Changed] - Move use_flipper logic inside use_react_native and simplify the Flipper dependencies logic

Test Plan

Executed a pod install with and without flipper and with isProduction true

analysis-bot · 2022-05-20T18:01:29Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 406a474
Branch: main

…use_flipper_into_use_react_native

cipolleschi

Thanks for tackling this issue. There are a couple of things to still take care of.

For context, this PR has been triggered by #33764, while we were investigating how to solve it.

Perhaps, it make sense to specify this into the PR description, WDYT?

packages/rn-tester/Podfile

scripts/react_native_pods.rb

f-meloni · 2022-05-23T08:22:20Z

Thanks for tackling this issue. There are a couple of things to still take care of.

For context, this PR has been triggered by #33764, while we were investigating how to solve it.

Perhaps, it make sense to specify this into the PR description, WDYT?

I'm still in the draft phase, I wanted to highlight the workaround that fixes #33764 when this is becomes ready for review, but I can add it now :)

f-meloni · 2022-05-23T08:39:09Z

scripts/react_native_pods.rb

@@ -108,6 +108,11 @@ def use_react_native! (options={})
    pod 'libevent', '~> 2.1.12'
  end

+  if flipper_configuration.flipper_enabled && !production


This is a temporary workaround that should fix #33764
If production is true the flipper pods are not added at all (differently than what CocoaPods does that compiles them, but then doesn't copy the frameworks), this avoids Flipper to be compiled when releasing, hence avoids the error discussed in the issue.
The downside of this workaround is that before making a release, the user needs to call pod install again

The downside of this workaround is that before making a release, the user needs to call pod install again

Correct me if I'm wrong, but it used to have to call it anyway, passing production = false, right?

I would also specify, perhaps also adding some doc, that it's still the case. When a user wants to build for release, it has to run PRODUCTION=1 pod install

That is correct, however if there wasn't the mentioned issue we could only rely on CocoaPods' configurations logic to do esclude Flipper from production (it would anyway compile flipper but wouldn't copy it) and remove the production flag.
The PRODUCTION=1 pod install is how this is set up on RNTester, final users could potentially set their own way to define if is production or not depending on their needs and their workflow.
If we think setting the PRODUCTION env variable is a good default to express that we want the production flag true I can add that to the template as well.

Can you add a comment about that here? I think this can be easily missed since it's such a weird quirk of CocoaPods.

Yes, sorry I missed this comment, will make another PR to fix those two comments as well

Opened #33902

cipolleschi

Thanks again for this. Added a clarification

cipolleschi · 2022-05-23T09:04:53Z

scripts/react_native_pods.rb

@@ -108,6 +108,11 @@ def use_react_native! (options={})
    pod 'libevent', '~> 2.1.12'
  end

+  if flipper_configuration.flipper_enabled && !production


The downside of this workaround is that before making a release, the user needs to call pod install again

Correct me if I'm wrong, but it used to have to call it anyway, passing production = false, right?

I would also specify, perhaps also adding some doc, that it's still the case. When a user wants to build for release, it has to run PRODUCTION=1 pod install

packages/rn-tester/Podfile

scripts/cocoapods/flipper.rb

scripts/react_native_pods.rb

template/ios/Podfile

Co-authored-by: Lorenzo Sciandra <notkelset@kelset.dev>

facebook-github-bot · 2022-05-23T10:47:35Z

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

cortinico · 2022-05-23T14:22:41Z

template/ios/Podfile

-  #
-  # Note that if you have use_frameworks! enabled, Flipper will not work and
-  # you should disable the next line.
-  use_flipper!()


Just a small heads up: shouldn't we deprecate use_flipper() now?

What happens to a user that is doing?

use_react_native!( ... :flipper_configuration => FlipperConfiguration.enabled, ) use_flipper!()

The idea is that it won't work, the function use_flipper won't exist anymore after this change.
The only thing is probably how tell the user that the flipper configuration is now into the use_react_native function.
In case we wanted to do so we could use something like

def use_flipper!(versions = {}, configurations: ['Debug']) Pod::UI.warn "use_flipper is deprecated, use the flipper_configuration option in the use_react_native function" use_flipper_pods(versions, :configurations => configurations) end

Yup that makes total sense 👍

cortinico · 2022-05-23T19:04:39Z

[iOS] [Changed] - Move use_flipper logic inside use_react_native and simplify the Flipper dependencies logic

Just a heads up that this should be flagged in the release notes for .70

facebook-github-bot · 2022-05-24T08:21:09Z

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

…use_flipper_into_use_react_native

facebook-github-bot · 2022-05-24T09:02:18Z

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

tido64 · 2022-05-24T10:27:16Z

scripts/cocoapods/flipper.rb

+    def self.enabled(configurations = ["Debug"], versions = {})
+        return FlipperConfiguration.new(true, configurations, versions)
+    end
+
+    def self.disabled
+        return FlipperConfiguration.new(false, [], {})
+    end


Nit: In Ruby, the last statement is always returned so we can drop return here to be more idiomatic:

Suggested change

def self.enabled(configurations = ["Debug"], versions = {})

return FlipperConfiguration.new(true, configurations, versions)

end

def self.disabled

return FlipperConfiguration.new(false, [], {})

end

def self.enabled(configurations = ["Debug"], versions = {})

FlipperConfiguration.new(true, configurations, versions)

end

def self.disabled

FlipperConfiguration.new(false, [], {})

end

tido64 · 2022-05-24T10:35:57Z

scripts/react_native_pods.rb

@@ -108,6 +108,11 @@ def use_react_native! (options={})
    pod 'libevent', '~> 2.1.12'
  end

+  if flipper_configuration.flipper_enabled && !production


Can you add a comment about that here? I think this can be easily missed since it's such a weird quirk of CocoaPods.

react-native-bot · 2022-05-24T11:45:05Z

This pull request was successfully merged by @f-meloni in 0bd5239.

^{When will my fix make it into a release? | Upcoming Releases}

…abled (#33902) Summary: Follow up of #33882 ## Changelog [Internal] - Add comment to explain why production flag is used when Flipper is enabled Pull Request resolved: #33902 Reviewed By: cortinico, f-meloni Differential Revision: D36632238 Pulled By: cipolleschi fbshipit-source-id: a859006851d9f50a4ad0ae1141006e8dac7aee6e

Issue - reactwg/react-native-releases#21 (comment) - facebook/react-native#33764 PR - facebook/react-native#33882

Summary: ### Mentioned - pr[main]: #33882 - discussion: reactwg/react-native-releases#21 (reply in thread) - pr[0.69-stable]: #34098 Close: #33764 Saw the issue ago couple wks too: leotm/react-native-template-new-architecture#757 Fixed similarly: leotm/react-native-template-new-architecture#791 ## Changelog [iOS] [Changed] - Update Podfile to allow `PRODUCTION=1 pod install` [CATEGORY] [TYPE] - Message Pull Request resolved: #34234 Test Plan: Everything builds and runs as expected Reviewed By: cortinico Differential Revision: D38029117 Pulled By: cipolleschi fbshipit-source-id: bdb58200a999cb66f1043a2feb670f9037c8e463

* Draft ios.yml * Fix indentation * Update ios.yml * Build app for release * Fix prod build error w Flipper, set env flag Issue - reactwg/react-native-releases#21 (comment) - facebook/react-native#33764 PR - facebook/react-native#33882 * Remove stale comment * Temporarily add `npx react-native info` * Move `npx react-native info` after `yarn` * Add release bundle step before build * Update bundle script * Update bundle script * Update RN CLI from next (8.0.0-alpha.0) to 9.0.0-alpha.5 * Remove devDep @react-native-community/cli No luck with 9.0.0-alpha.5 Should now use npm:9.0.0-alpha.4 (via npm:^9.0.0-alpha.3) * Try bundling also to ios/MyApp/main.jsbundle * Add debug before release bundle/build * Fix yarn lockfile/cache conflict res * Add normal pod install before debug * Comment debug pod install and build, working fine * Update pbxproj and xcscheme Follow-up: - facebook/react-native#34274 (comment) Also update parts in-line with current main - https://github.com/facebook/react-native/tree/main/template * Disable Metro experimentalImportSupport * Resolve Yarn lockfile From conflict res * Rename xcode.env prefix _ to . * Remove react-native-flipper * Revert "Remove react-native-flipper" This reverts commit 646dd74. * Revert "Rename xcode.env prefix _ to ." This reverts commit 8f15810. * Revert "Disable Metro experimentalImportSupport" This reverts commit 41ed14b. * Add index.js for iOS Release bundling * Add back @react-native-community/cli 9.0.0-alpha.5 * Revert "Try bundling also to ios/MyApp/main.jsbundle" This reverts commit 80e7354. * Revert "Comment debug pod install and build, working fine" This reverts commit 0ea4d01. * Update workflow comments

Move use flipper logic inside use_react_native

c05cb76

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: Facebook Partner: Facebook Partner labels May 20, 2022

react-native-bot added the Platform: iOS iOS applications. label May 20, 2022

f-meloni added 2 commits May 20, 2022 19:40

Add configuration for flipper

c3d8783

Fix tests

fdd92c8

Merge branch 'main' of https://github.com/facebook/react-native into …

387d626

…use_flipper_into_use_react_native

cipolleschi requested changes May 23, 2022

View reviewed changes

packages/rn-tester/Podfile Show resolved Hide resolved

scripts/react_native_pods.rb Outdated Show resolved Hide resolved

scripts/react_native_pods.rb Outdated Show resolved Hide resolved

Disable flipper if USE_FRAMEWORK is enabled

0bc843f

Add production flag

7be8a3e

f-meloni commented May 23, 2022

View reviewed changes

Update template

3e9545f

cipolleschi approved these changes May 23, 2022

View reviewed changes

f-meloni mentioned this pull request May 23, 2022

Unable to build release version for iOS with the new architecture #33764

Closed