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.

Sign up for GitHub

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

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

Closed
wants to merge 11 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c05cb76
Move use flipper logic inside use_react_native
f-meloni May 20, 2022
c3d8783
Add configuration for flipper
f-meloni May 20, 2022
fdd92c8
Fix tests
f-meloni May 20, 2022
387d626
Merge branch 'main' of https://github.com/facebook/react-native into …
f-meloni May 23, 2022
0bc843f
Disable flipper if USE_FRAMEWORK is enabled
f-meloni May 23, 2022
7be8a3e
Add production flag
f-meloni May 23, 2022
3e9545f
Update template
f-meloni May 23, 2022
543f1d8
Fix typo
f-meloni May 23, 2022
b2d71d5
Fix space from previous commit
f-meloni May 23, 2022
b3a0bc6
Add warning for use_flipper
f-meloni May 23, 2022
dc8ad2e
Merge branch 'main' of https://github.com/facebook/react-native into …
f-meloni May 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions packages/rn-tester/Podfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,10 @@ def pods(options = {})
path: @prefix_path,
fabric_enabled: fabric_enabled,
hermes_enabled: hermes_enabled,
flipper_configuration: USE_FRAMEWORKS ? FlipperConfiguration.disabled : FlipperConfiguration.enabled,
app_path: "#{Dir.pwd}",
config_file_dir: "#{Dir.pwd}/node_modules",
production: !ENV['PRODUCTION'].nil?
)
pod 'ReactCommon/turbomodule/samples', :path => "#{@prefix_path}/ReactCommon"

Expand All @@ -48,9 +50,6 @@ end

target 'RNTester' do
pods()
if !USE_FRAMEWORKS
cipolleschi marked this conversation as resolved.
Show resolved Hide resolved
use_flipper!
end
end

target 'RNTesterUnitTests' do
Expand Down
13 changes: 2 additions & 11 deletions scripts/cocoapods/__tests__/flipper-test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,24 +16,15 @@ def setup
# =========================== #
# TEST - Install Dependencies #
# =========================== #
def test_installFlipperDependencies_whenProductionIsFalse_installDependencies
def test_installFlipperDependencies_installDependencies
# Act
install_flipper_dependencies(false, '../..')
install_flipper_dependencies('../..')

# Assert
assert_equal($podInvocationCount, 1)
assert_equal($podInvocation['React-Core/DevSupport'][:path], "../../" )
end

def test_installFlipperDependencies_whenProductionIsTrue_skipDependencies
# Act
install_flipper_dependencies(true, '../..')

# Assert
assert_equal($podInvocationCount, 0)
assert_true($podInvocation.empty?)
end

# ======================= #
# TEST - Use Flipper Pods #
# ======================= #
Expand Down
26 changes: 22 additions & 4 deletions scripts/cocoapods/flipper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,8 @@
#
# @parameter production: a boolean that indicates whether we are in production or not.
# @parameter pathToReactNative: the path to the React Native installation
def install_flipper_dependencies(production, pathToReactNative)
unless production
pod 'React-Core/DevSupport', :path => "#{pathToReactNative}/"
end
def install_flipper_dependencies(pathToReactNative)
pod 'React-Core/DevSupport', :path => "#{pathToReactNative}/"
end


Expand Down Expand Up @@ -92,3 +90,23 @@ def flipper_post_install(installer)
end
end
end

class FlipperConfiguration
attr_reader :flipper_enabled
attr_reader :configurations
attr_reader :versions

def initialize(flipper_enabled, configurations, versions)
@flipper_enabled = flipper_enabled
@configurations = configurations
@versions = versions
end

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

def self.disabled
return FlipperConfiguration.new(false, [], {})
end
Comment on lines +105 to +111
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end
10 changes: 8 additions & 2 deletions scripts/react_native_pods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ def use_react_native! (options={})
# Include Hermes dependencies
hermes_enabled = options[:hermes_enabled] ||= false

flipper_configuration = options[:flipper_configuration] ||= FlipperConfiguration.disabled

if `/usr/sbin/sysctl -n hw.optional.arm64 2>&1`.to_i == 1 && !RUBY_PLATFORM.include?('arm64')
Pod::UI.warn 'Do not use "pod install" from inside Rosetta2 (x86_64 emulation on arm64).'
Pod::UI.warn ' - Emulated x86_64 is slower than native arm64'
Expand All @@ -59,8 +61,6 @@ def use_react_native! (options={})
pod 'React-RCTVibration', :path => "#{prefix}/Libraries/Vibration"
pod 'React-Core/RCTWebSocket', :path => "#{prefix}/"

install_flipper_dependencies(production, prefix)

pod 'React-bridging', :path => "#{prefix}/ReactCommon/react/bridging"
pod 'React-cxxreact', :path => "#{prefix}/ReactCommon/cxxreact"
pod 'React-jsi', :path => "#{prefix}/ReactCommon/jsi"
Expand Down Expand Up @@ -108,6 +108,11 @@ def use_react_native! (options={})
pod 'libevent', '~> 2.1.12'
end

if flipper_configuration.flipper_enabled && !production
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@f-meloni f-meloni May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@f-meloni f-meloni May 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #33902

install_flipper_dependencies(prefix)
use_flipper_pods(flipper_configuration.versions, :configurations => flipper_configuration.configurations)
end

pods_to_update = LocalPodspecPatch.pods_to_update(options)
if !pods_to_update.empty?
if Pod::Lockfile.public_instance_methods.include?(:detect_changes_with_podfile)
Expand All @@ -122,6 +127,7 @@ def get_default_flags()
flags = {
:fabric_enabled => false,
:hermes_enabled => false,
:flipper_configuration => FlipperConfiguration.disabled
}

if ENV['RCT_NEW_ARCH_ENABLED'] == '1'
Expand Down
11 changes: 5 additions & 6 deletions template/ios/Podfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,11 @@ target 'HelloWorld' do
# You can enabled/disable it manually by replacing `flags[:hermes_enabled]` with `true` or `false`.
:hermes_enabled => flags[:hermes_enabled],
:fabric_enabled => flags[:fabric_enabled],
# Enables Flipper.
#
# Note that if you have use_frameworks! enabled, Flipper will not work and
# you should disable the next line.
:flipper_configuration => FlipperConfiguration.enabled,
# An absolute path to your application root.
:app_path => "#{Pod::Config.instance.installation_root}/.."
)
Expand All @@ -25,12 +30,6 @@ target 'HelloWorld' do
# Pods for testing
end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!()

Copy link
Contributor Author

@f-meloni f-meloni May 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup that makes total sense 👍


post_install do |installer|
react_native_post_install(installer)
__apply_Xcode_12_5_M1_post_install_workaround(installer)
Expand Down