-
Notifications
You must be signed in to change notification settings - Fork 8
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
Compatibility Changes for React Native 0.69.2+ (SDK46) #20
Conversation
plugin/src/withFlipper.ios.ts
Outdated
? `FlipperConfiguration.enabled(["Debug"], { 'Flipper' => '${version}' }),` | ||
: `FlipperConfiguration.enabled,`; | ||
const inactive = `FlipperConfiguration.disabled`; | ||
return `:flipper_configuration => ENV['FLIPPER_DISABLE'] ? ${inactive} : ${active}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean by default it should be installing flipper cocoapods? Or do we have to specify FLIPPER_DISABLE=0 in our environment variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears from the react-native code that as of 0.69.2+ Flipper's always included by default, but is included conditionally inside of use_react_native
instead of as a separate use_flipper
directive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I'll probably do is change this to a hard-check for == "1" similar to the production check that's also needed as part of the 0.69.2 changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. So this part of the code should technically be automatically uncommented from expo-community-flipper plugin when I don't assign FLIPPER_DISABLE=1 or if it doesn't find any value for FLIPPER_DISABLE from environment variables?
This is from the generated Podfile
# Uncomment to opt-in to using Flipper
# Note that if you have use_frameworks! enabled, Flipper will not work
#
# if !ENV['CI']
# use_flipper!()
# end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An important note: If you're doing a production build of react-native, flipper is automatically disabled by react-native and should be skipped. This is the other reason we needed to ensure the production flag was passed in to use_react_native
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea. I'd assume we want to be able have FLIPPER_DISABLE=1 only in staging and production expo pipeline as a secret. And locally we woudldn't have to assign this environment variable anything to get flipper to build with flipper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. So this part of the code should technically be automatically uncommented from expo-community-flipper plugin
Weirdly, no. When we expo prebuild, the expo template has the old flipper code from pre 0.69.2, which was the source of a lot of the SDK 46 pain. https://github.com/react-native-community/react-native-template-typescript/blob/main/template/ios/Podfile is the Podfile we want to reference for enabling Flipper, since its based on the latest RN code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in addition to FLIPPER_DISABLE=1 I'd have to ensure that in our expo build pipeline that PRODUCTION=1 is also set as an environment variable?
It’s probably not required, but I would take chances after the breaking
patch change. I’ll update the readme in the pr to reflect this. I also need
to dive into the use_react_native podfile and see how the production arg is
used.
…On Tue, Aug 23 2022 at 7:51 PM, rlam3 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In plugin/src/withFlipper.ios.ts
<#20 (comment)>
:
> + IOS_URN_ARG_ANCHOR,
+} from "./constants";
+import { FlipperConfig } from "./types";
+import path from "path";
+import fs from "fs";
+
+/** Create a namepaced tag */
+const tag = (s: string) => `${EXPO_FLIPPER_TAG}-${s}`;
+
+/** Return the flipper enabling line for use inside of use_react_native */
+const createFlipperArgument = (version?: string) => {
+ const active = version
+ ? `FlipperConfiguration.enabled(["Debug"], { 'Flipper' => '${version}' }),`
+ : `FlipperConfiguration.enabled,`;
+ const inactive = `FlipperConfiguration.disabled`;
+ return `:flipper_configuration => ENV['FLIPPER_DISABLE'] ? ${inactive} : ${active}`;
So in addition to FLIPPER_DISABLE=1 I'd have to ensure that in our expo
build pipeline that PRODUCTION=1 is also set as an environment variable?
—
Reply to this email directly, view it on GitHub
<#20 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAOA2QLOAEBQBYTG5UC63V2WEZRANCNFSM57KRCS4Q>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
From experimenting with eas builds via expo. It seems like if we set FLIPPER_DISABLE=1 as a secret in
I was able to verify that flipper was not installed via the "Install Pods" section of the expo build pipeline and I was able to download the successful build and install it via iOS simulator to verify that Flipper desktop app was not able to connect the app itself. The final piece I'd like to tackle now is how to separate FLIPPER_DISABLE=1 in prod and staging but FLIPPER_DISABLE=0 in local and development. I think we can merge this PR! |
This should do it: "production": {
"extends": "base",
"distribution": "store",
"releaseChannel": "stable",
"android": {
"buildType": "app-bundle"
},
"env": {
"FLIPPER_DISABLE": "true"
}
} |
@slapbox thank you! is it "true" or is it supposed to be "1" ? |
I think either should work, just be sure to wrap the value in quotes. |
Set it to "1" to disable it in your production profile. Leave it unset for development/testing profiles. "production": {
"extends": "base",
"distribution": "store",
"releaseChannel": "stable",
"android": {
"buildType": "app-bundle"
},
"env": {
"PRODUCTION": "1", // not required necessarily, but useful for react-native's bundler
"FLIPPER_DISABLE": "1"
}
} |
This will likely address the issues in #18 and #19.
Context: In React Native 0.69.2, the way flipper was added to the build for iOS was changed in a backward incompatible way. Previously, flipper was invoked with a
use_flipper
invocation in the Podfile. The upgrade helper shows that the file changed significantly at the 0.69.2 release of react nativeuse_react_native
and the olduse_flipper
is removedpost_install
step is no longer requiredThe result is inconsistent builds on iOS, where the iOS build will fail while Android succeeds. Forcing old versions of flipper has limited success, but isn't a good long term solution.
How to Test This Change
/plugin/build
folder into your expo appYou can then run
expo prebuild
and verify the new Podfile changes.expo-community-flipper-isprod
tag that tellsuse_react_native
if the production ENV value was setexpo-community-flipper-urn
tag that tellsuse_react_native
to enable/disable flipper based on theFLIPPER_DISABLE
environment variable