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

Jump to bottom

Upgrade to React Native v63. #4420

Merged
merged 7 commits into from
Jan 22, 2021
Merged

Upgrade to React Native v63. #4420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d346073
ComposeBox [nfc]: Say a more accurate thing in a TODO.
chrisbobbe Jan 21, 2021
17d9285
ComposeBox [nfc]: Un-export a function that's clearly local.
chrisbobbe Jan 21, 2021
df56386
types: Relocate some `$FlowFixMe`s and correct their TODOs.
chrisbobbe Jan 21, 2021
86e6349
deps: Point to PR commit for @react-native-community/cameraroll.
chrisbobbe Jan 21, 2021
bcc1356
deps: Upgrade to React Native v0.63.
chrisbobbe Jan 19, 2021
6d6a3d8
podfile [nfc]: Set up Flipper Pods by calling a function exposed by RN.
chrisbobbe Jan 19, 2021
0119399
presenceReducer [nfc]: Point to the right Flow bug in two fixmes.
chrisbobbe Jan 21, 2021
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
2 changes: 1 addition & 1 deletion .flowconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,4 @@ suppress_comment=\\(.\\|\n\\)*\\$FlowExpectedError
suppress_comment=\\(.\\|\n\\)*\\$FlowMigrationFudge

[version]
^0.113.0
^0.122.0
76 changes: 3 additions & 73 deletions ios/Podfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,87 +2,17 @@
# (project > ZulipMobile > Info in Xcode)
platform :ios, '11.0'

require_relative '../node_modules/react-native/scripts/react_native_pods'
require_relative '../node_modules/@react-native-community/cli-platform-ios/native_modules'
require_relative '../node_modules/react-native-unimodules/cocoapods.rb'

# Add Flipper Pods
def add_flipper_pods!(versions = {}, configurations: ['Debug'])
versions['Flipper'] ||= '~> 0.54.0'
versions['Flipper-DoubleConversion'] ||= '1.1.7'
versions['Flipper-Folly'] ||= '~> 2.2'
versions['Flipper-Glog'] ||= '0.3.6'
versions['Flipper-PeerTalk'] ||= '~> 0.0.4'
versions['Flipper-RSocket'] ||= '~> 1.1'

pod 'FlipperKit', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/FlipperKitLayoutPlugin', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/SKIOSNetworkPlugin', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/FlipperKitUserDefaultsPlugin', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/FlipperKitReactPlugin', versions['Flipper'], :configurations => configurations

# List all transitive dependencies for FlipperKit pods
# to avoid them being linked in Release builds
pod 'Flipper', versions['Flipper'], :configurations => configurations
pod 'Flipper-DoubleConversion', versions['Flipper-DoubleConversion'], :configurations => configurations
pod 'Flipper-Folly', versions['Flipper-Folly'], :configurations => configurations
pod 'Flipper-Glog', versions['Flipper-Glog'], :configurations => configurations
pod 'Flipper-PeerTalk', versions['Flipper-PeerTalk'], :configurations => configurations
pod 'Flipper-RSocket', versions['Flipper-RSocket'], :configurations => configurations
pod 'FlipperKit/Core', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/CppBridge', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/FBCxxFollyDynamicConvert', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/FBDefines', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/FKPortForwarding', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/FlipperKitHighlightOverlay', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/FlipperKitLayoutTextSearchable', versions['Flipper'], :configurations => configurations
pod 'FlipperKit/FlipperKitNetworkPlugin', versions['Flipper'], :configurations => configurations
end

# Post Install processing for Flipper
def flipper_post_install(installer)
installer.pods_project.targets.each do |target|
if target.name == 'YogaKit'
target.build_configurations.each do |config|
config.build_settings['SWIFT_VERSION'] = '4.1'
end
end
end
end

target 'ZulipMobile' do
# Pods from React Native
pod 'FBLazyVector', :path => "../node_modules/react-native/Libraries/FBLazyVector"
pod 'FBReactNativeSpec', :path => "../node_modules/react-native/Libraries/FBReactNativeSpec"
pod 'RCTRequired', :path => "../node_modules/react-native/Libraries/RCTRequired"
pod 'RCTTypeSafety', :path => "../node_modules/react-native/Libraries/TypeSafety"
use_react_native!

pod 'React', :path => '../node_modules/react-native/'
pod 'React-Core', :path => '../node_modules/react-native/'
pod 'React-CoreModules', :path => '../node_modules/react-native/React/CoreModules'
pod 'React-Core/DevSupport', :path => '../node_modules/react-native/'
Copy link
Member

Choose a reason for hiding this comment

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

One change here is that this particular pod no longer gets included. The upstream function has this:

  unless production
    pod 'React-Core/DevSupport', :path => "#{prefix}/"
  end

and production defaults to false.

I'm not sure what that does. It sounds like something that's good to leave out in release builds, but we might miss in dev builds. But I guess if things still seem to work well in development, then we didn't need it?

Copy link
Contributor Author

@chrisbobbe chrisbobbe Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Since production defaults to false, and we don't take it away from the default (which we would do by passing an argument with our use_react_native! call in the Podfile), I think this pod is still included in all builds.

  unless false
    // This code will definitely run
  end

unless false is equivalent to if true, I think. Is that right?

Copy link
Contributor Author

@chrisbobbe chrisbobbe Jan 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I've just changed it to unless true in node_modules and run pod install (via yarn), and I see a line with React-Core/DevSupport removed from Podfile.lock.

(The / says to look for DevSupport as a "subspec" of React-Core. I think all subspecs of React-Core would normally have been pulled in by the pod 'React-Core' line, above this conditional—but this isn't happening here because React-Core's podspec declares a "default subspec"; doc.)

Probably best to keep pulling in DevSupport for development, but I agree that it should be fine to exclude it in production builds. I've just tried doing a release build with it excluded, and it built with no errors and is so far running just fine on my iPhone 6s.

I'd be nice if React Native took the approach it took with use_flipper! in facebook/react-native@c72ecdb (see also our 61bda2a) toward use_react_native!. The "build configuration" (the thing we use at build time to say whether we're building for production or development) isn't known at pod install time, which is the time the Podfile is run. So I don't see a way we could pass a correct value for the production param that use_react_native! expects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I don't see a way we could pass a correct value for the production param that use_react_native! expects.

Given this, it's possibly just fine to not do anything for now. Including React-Core/DevSupport in release builds is the status quo anyway.

Copy link
Member

Choose a reason for hiding this comment

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

unless false is equivalent to if true, I think. Is that right?

Ha, yes, I guess I misread that code 🙃

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree with all the above. This change is NFC -- we were always getting this pod, and we're still always getting it. And the appropriate thing would be for that function to do something like what use_flipper! does, and take a list of configurations for it to apply in.

(Or else have some other way of being conditioned on the configuration.)

pod 'React-RCTActionSheet', :path => '../node_modules/react-native/Libraries/ActionSheetIOS'
pod 'React-RCTAnimation', :path => '../node_modules/react-native/Libraries/NativeAnimation'
pod 'React-RCTBlob', :path => '../node_modules/react-native/Libraries/Blob'
pod 'React-RCTImage', :path => '../node_modules/react-native/Libraries/Image'
pod 'React-RCTLinking', :path => '../node_modules/react-native/Libraries/LinkingIOS'
pod 'React-RCTNetwork', :path => '../node_modules/react-native/Libraries/Network'
pod 'React-RCTSettings', :path => '../node_modules/react-native/Libraries/Settings'
pod 'React-RCTText', :path => '../node_modules/react-native/Libraries/Text'
pod 'React-RCTVibration', :path => '../node_modules/react-native/Libraries/Vibration'
pod 'React-Core/RCTWebSocket', :path => '../node_modules/react-native/'
pod 'React-cxxreact', :path => '../node_modules/react-native/ReactCommon/cxxreact'
pod 'React-jsi', :path => '../node_modules/react-native/ReactCommon/jsi'
pod 'React-jsiexecutor', :path => '../node_modules/react-native/ReactCommon/jsiexecutor'
pod 'React-jsinspector', :path => '../node_modules/react-native/ReactCommon/jsinspector'
# This is deprecated upstream; removing it is #4115. See
# https://reactnative.dev/docs/pushnotificationios.
pod 'React-RCTPushNotification', :path => '../node_modules/react-native/Libraries/PushNotificationIOS'
pod 'ReactCommon/callinvoker', :path => "../node_modules/react-native/ReactCommon"
pod 'ReactCommon/turbomodule/core', :path => "../node_modules/react-native/ReactCommon"
pod 'Yoga', :path => '../node_modules/react-native/ReactCommon/yoga', :modular_headers => true
pod 'DoubleConversion', :podspec => '../node_modules/react-native/third-party-podspecs/DoubleConversion.podspec'
pod 'glog', :podspec => '../node_modules/react-native/third-party-podspecs/glog.podspec'
pod 'Folly', :podspec => '../node_modules/react-native/third-party-podspecs/Folly.podspec'

# "Autolinking": automatically link pods that depend on React
# Native, unless omitted in our own `react-native.config.js`.
Expand All @@ -92,7 +22,7 @@ target 'ZulipMobile' do
#
# Note that if you have use_frameworks! enabled, Flipper will not work and
# you should disable these next few lines.
add_flipper_pods!
use_flipper!
post_install do |installer|
flipper_post_install(installer)
end
Expand Down
Loading