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

Flipper for React Native 0.62 #27565

Closed
rickhanlonii opened this issue Dec 18, 2019 · 31 comments
Closed

Flipper for React Native 0.62 #27565

rickhanlonii opened this issue Dec 18, 2019 · 31 comments

Comments

@rickhanlonii
Copy link
Member

@rickhanlonii rickhanlonii commented Dec 18, 2019

Overview

This issue is to track items to get Flipper enabled for 0.62.

RNTester Integration

  • Android works with Flipper in RNTester
  • iOS works with Flipper in RNTester

Default Template Integration

  • Android - Can install app from template and use Flipper by default.
  • iOS - Can install app from template and use Flipper by default

Upgrade Flipper SDK Version

Upgrading

  • Address pod install speed with openssl Edit: addressed here
  • Address swift inclusion
  • Clean up upgrade requirements
  • Address React Devtools issue
    • React devtools plugin for Flipper is blank
  • Make sure the upgrade tool works well for this release #27879
  • Figure out use_framworks support

Documentation

  • Add React Native the Flipper website
  • Add documentation to the website
    • Adding Flipper to new projects (mention included by default after 0.62)
    • Adding Flipper to existing projects
  • In Flipper Docs

Commits to Cherry Pick

@react-native-bot

This comment was marked as outdated.

Copy link
Collaborator

@react-native-bot react-native-bot commented Dec 18, 2019

We are automatically closing this issue because it does not appear to follow any of the provided issue templates.

👉 Click here if you want to report a reproducible bug or regression in React Native.

@TheSavior

This comment has been minimized.

Copy link
Member

@TheSavior TheSavior commented Dec 19, 2019

I give up. @hramos! 😭

@rmevans9

This comment has been minimized.

Copy link
Contributor

@rmevans9 rmevans9 commented Dec 19, 2019

While working on a Flipper plugin it was noted that the inclusion of FlipperKit in the Podfile caused pods to need to install openssl-ios-bitcode which required building openssl for all the architectures. As you can imagine this takes a while. In a normal pod install the developer will not know what is going on likely as this is what they will see for a long time:

Screen Shot 2019-12-18 at 8 12 14 PM

If you run pod install --verbose you can actually see progress during this process. On my machine it took a good 10+ minutes to complete this install. The good news is once this pod is in your cache future pod installs are much quicker. If you would like to experience this first hand you can do the following steps:

  1. Run pod cache clear openssl-ios-bitcode
  2. Run pod install (include --verbose if you are interested in seeing what it is actually doing) in a ios folder of a RN project with the FlipperKit pods setup. If Pods have been installed previously do a rm -rf Pods/ for good measure too.

I understand the need for this library but it will be a jarring experience for developers upgrading and also for first time users of react-native. I feel like if we can't avoid it that it is at least worth calling attention to it so users don't cancel the pod install part way through and say its broken.

Edit: It seems maybe the openssl-ios-bitcode dependency changed here https://github.com/facebook/flipper/pull/577/files so maybe this is a non issue with the later versions of Flipper. My experience is based upon the Flipper version that was floating around the rn repository a while back - 0.23.6

@TheSavior

This comment has been minimized.

Copy link
Member

@TheSavior TheSavior commented Dec 19, 2019

@rmevans9, I added that to the checklist. 👍

@charpeni

This comment has been minimized.

Copy link
Collaborator

@charpeni charpeni commented Dec 19, 2019

@rmevans9 thanks for bringing that.

I confirm that bumping the version from 0.23.6 to 0.30.0 solved that issue, we now rely on OpenSSL-Universal (1.0.2.19) instead of openssl-ios-bitcode. I've just double checked, and openssl-ios-bitcode is not in Podfile.lock.

@charpeni

This comment has been minimized.

Copy link
Collaborator

@charpeni charpeni commented Dec 19, 2019

Looks like I got you, bot!

@rmevans9

This comment has been minimized.

Copy link
Contributor

@rmevans9 rmevans9 commented Dec 19, 2019

@charpeni ok perfect! I am glad that issue got solved with the upgrade.

@safaiyeh

This comment has been minimized.

Copy link
Collaborator

@safaiyeh safaiyeh commented Dec 19, 2019

Submitted a PR to initialize Flipper on Android & iOS based on the context from @axe-fb.

PR: #27569

Conversation: react-native-community/releases#145 (comment)

facebook-github-bot added a commit that referenced this issue Feb 3, 2020
Summary:
Related to #27426 & #27565.

The wish to not include an empty Swift file to trigger the correct Xcode settings was voiced in #27426 & #27565.

## Changelog

[iOS] [Fixed] - Remove need for Swift file in the user’s project in order to use Flipper
Pull Request resolved: #27922

Test Plan:
An application created with the template, like so:

```bash
react-native init --template=~/Code/React/react-native TestFlipper
```

…will successfully build, launch, and have Flipper connect.

Differential Revision: D19690592

Pulled By: cpojer

fbshipit-source-id: ee696e0d747d6338534b0c2d62029e64ece02cd3
@rickhanlonii

This comment has been minimized.

Copy link
Member Author

@rickhanlonii rickhanlonii commented Feb 5, 2020

@alloy that all seems to make sense to me and seems like a safer option for upgrades, want to send the PRs?

rickhanlonii added a commit that referenced this issue Feb 5, 2020
Summary:
0.62 Flipper Support items: #27565
Made RNTester's Android Flipper implementation consistent with the template.
~~Added Flipper to the iOS build.~~
<img width="1259" alt="Screen Shot 2019-12-28 at 7 06 42 PM" src="https://user-images.githubusercontent.com/8675043/71551835-37290800-29a5-11ea-9eac-b119a69a68c1.png">

## Changelog

[Internal] [Added] - RNTester Android Fipper updates
Pull Request resolved: #27631

Test Plan: Run RNTester and see if it connects to Flipper.

Reviewed By: rickhanlonii

Differential Revision: D19345093

Pulled By: passy

fbshipit-source-id: 6957c1ca3f4a5bb7f0e581c5daf8ddeac5d87eea
rickhanlonii added a commit that referenced this issue Feb 5, 2020
Summary:
Issue: #27565

initalizeFlipper should be set in template app by default.

## Changelog

[iOS] [Changed] - Added Flipper to template app
[Android] [Changed] - Added Flipper to template app
Pull Request resolved: #27569

Test Plan:
Connect Flipper to the iOS application
Connect Flipper to the Android application

Reviewed By: passy

Differential Revision: D19344704

Pulled By: rickhanlonii

fbshipit-source-id: ca126fd2caab13751ddc2ce6d195bd0c644c704e
alloy added a commit to alloy/react-native that referenced this issue Feb 5, 2020
Summary:
Related to facebook#27426 & facebook#27565.

The wish to not include an empty Swift file to trigger the correct Xcode settings was voiced in facebook#27426 & facebook#27565.

[iOS] [Fixed] - Remove need for Swift file in the user’s project in order to use Flipper
Pull Request resolved: facebook#27922

Test Plan:
An application created with the template, like so:

```bash
react-native init --template=~/Code/React/react-native TestFlipper
```

…will successfully build, launch, and have Flipper connect.

Differential Revision: D19690592

Pulled By: cpojer

fbshipit-source-id: ee696e0d747d6338534b0c2d62029e64ece02cd3
@rickhanlonii

This comment has been minimized.

Copy link
Member Author

@rickhanlonii rickhanlonii commented Feb 5, 2020

Ok I ran some testing and:

RNTester

  • Android works
  • iOS does not work (PR open) 🚫

Default template

  • Android works
  • iOS works
@alloy

This comment has been minimized.

Copy link
Collaborator

@alloy alloy commented Feb 5, 2020

As mentioned above, a change was made in Flipper’s dependencies to switch from the openssl-ios-bitcode pod to OpenSSL-Universal in order to reduce build times. However, the OpenSSL-Universal pod does not include a bitcode slice, which both Apple’s and RN’s application templates enable by default for device builds (simulator builds are unaffected).

This means that, with the current RN template, users will not be able to build for device and have Flipper integrated at the same time.

Perhaps possible, but, as of yet, untested solutions:

  • Disable bitcode in Debug configurations in the RN template. (Unsure if there any cons to this.)
  • Use a different existing pod that does have prebuilt binaries for bitcode. (Unsure if one exists.)
  • Revert to building from source. (While this does take long initially, it does bring us closer to a setup where Flipper will be able to build with both static and dynamic libs without the need to monkey-patch CocoaPods.)
@priteshrnandgaonkar

This comment has been minimized.

Copy link
Contributor

@priteshrnandgaonkar priteshrnandgaonkar commented Feb 6, 2020

Disable bitcode in Debug configurations in the RN template. (Unsure if there any cons to this.)

I think we should try this approach, as Flipper is enabled only for Debug builds.

Use a different existing pod that does have prebuilt binaries for bitcode. (Unsure if one exists.)

I had looked at it last time, but didn't find any such prebuilt binaries.

Revert to building from source. (While this does take long initially, it does bring us closer to a setup where Flipper will be able to build with both static and dynamic libs without the need to monkey-patch CocoaPods.)

Not entirely sure about this approach, as the build times are long at the start, and due to which we have also faced some CI test's time outs and user reports regarding the long build times.

@priteshrnandgaonkar

This comment has been minimized.

Copy link
Contributor

@priteshrnandgaonkar priteshrnandgaonkar commented Feb 6, 2020

@alloy Sample, SampleSwift and Tutorial apps in Flipper repo are bitcode enabled and they compile. OpenSSL-Universal seems to be bitcode enabled, see this

@alloy

This comment has been minimized.

Copy link
Collaborator

@alloy alloy commented Feb 6, 2020

Well blimey, that’s great! I’ll double-check my build machine tomorrow to see why ld was complaining about this.

@alloy

This comment has been minimized.

Copy link
Collaborator

@alloy alloy commented Feb 7, 2020

Alas, seems not the case:

ld: '/Users/eloy/tmp/TestWithAllChanges/ios/Pods/OpenSSL-Universal/ios/lib/libcrypto.a(bio_lib.o)' does not contain bitcode. You must rebuild it with bitcode enabled (Xcode setting ENABLE_BITCODE), obtain an updated library from the vendor, or disable bitcode for this target. for architecture arm64

We can see that the slices have no bitcode section:

otool -l -arch arm64 ios/Pods/OpenSSL-Universal/ios/lib/libssl.a  | grep LLVM
otool -l -arch arm64 ios/Pods/OpenSSL-Universal/ios/lib/libcrypto.a | grep LLVM

…and indeed, looking at the last commits of the OpenSSL-Universal pod, bitcode has been disabled:

$ git log --pretty=oneline
17794e6faf5928f42264a7e3ef093ea7c08c5a15 (HEAD -> master, tag: 1.0.219, tag: 1.0.2.19, origin/master, origin/HEAD) Rebuild without bitcode
7ca5804216a079ce057d01502cc224c2a34642ea ENABLE_BITCODE = NO for iOS target
896c0ae5d5d10e93dd93789125ee12860df1ffb9 Add missing include
52ef14139ab791650ddad38ece782285ef56d722 Update FUNDING.yml
9e14bcdd27ae2323742cbdb8c221ae48ad1b19ac Merge pull request #74 from ilammy/disable-bitcode
5477325f53df56b92f672ea7c2399aed46c5e4fc Disable LLVM bitcode support (again)

On the flip side, I did experiment with changing the application to build without bitcode in Debug and that did successfully build and run. (The Flipper client applications finds the app running on my device, although it doesn’t seem to connect or detect any plugins, is that known? Just found facebook/flipper#262)

In any case, it seems like for now the best solution would be to indeed disabled bitcode in Debug builds by default in the RN application template.

@alloy

This comment has been minimized.

Copy link
Collaborator

@alloy alloy commented Feb 12, 2020

So regarding the need to use a preprocessor define to compile Flipper into the app, I did create react-native-community/cli#958 and alloy@ae4f936.

However, having thoroughly tested this manually, I came around to how this should work. It feels like we don’t need to take on this extra fragmented complexity; instead we just always define FB_SONARKIT_ENABLED in the Debug configuration and only wrap the AppDelegate.m code in a #if DEBUG.

People can always just remove the code from AppDelegate.m entirely, like they can the dependencies from their Podfile.

Put differently; imo we can just always enable Flipper by default in the template and keep things simple.

alloy added a commit to alloy/react-native that referenced this issue Feb 13, 2020
alloy added a commit to alloy/react-native that referenced this issue Feb 13, 2020
Flipper’s dependencies don’t all come with bitcode. See:
facebook#27565 (comment)
alloy added a commit to alloy/react-native that referenced this issue Feb 13, 2020
Flipper’s dependencies don’t all come with bitcode. See:
facebook#27565 (comment)
@alloy

This comment has been minimized.

Copy link
Collaborator

@alloy alloy commented Feb 13, 2020

Alright, #28044 should address all my open feedback.

facebook-github-bot added a commit that referenced this issue Feb 13, 2020
Summary:
Addresses my feedback [here](#27565 (comment)), [here](#27565 (comment)), and [here](#27565 (comment)).

## Changelog

[iOS] [Changed] - Updated Flipper iOS integration to be included by default in the `Debug` configuration
Pull Request resolved: #28044

Test Plan:
Manually tested that a new application from this template still works and that the Flipper integration works.

<img width="912" alt="Screenshot 2020-02-13 at 02 09 42" src="https://user-images.githubusercontent.com/2320/74391951-eb6fd800-4e05-11ea-9fde-7e0eb42c1ec4.png">

Differential Revision: D19871482

Pulled By: TheSavior

fbshipit-source-id: a805808fdd0c2dfdfe47dd59ffee02c81f3fdfa7
alloy added a commit that referenced this issue Feb 13, 2020
Summary:
Addresses my feedback [here](#27565 (comment)), [here](#27565 (comment)), and [here](#27565 (comment)).

## Changelog

[iOS] [Changed] - Updated Flipper iOS integration to be included by default in the `Debug` configuration
Pull Request resolved: #28044

Test Plan:
Manually tested that a new application from this template still works and that the Flipper integration works.

<img width="912" alt="Screenshot 2020-02-13 at 02 09 42" src="https://user-images.githubusercontent.com/2320/74391951-eb6fd800-4e05-11ea-9fde-7e0eb42c1ec4.png">

Differential Revision: D19871482

Pulled By: TheSavior

fbshipit-source-id: a805808fdd0c2dfdfe47dd59ffee02c81f3fdfa7
@todorone

This comment has been minimized.

Copy link

@todorone todorone commented Mar 3, 2020

Is Flipper can be already consumed in latest 0.62 RC? Super excited about it, thanks a lot to all contributors for working on integration.

@alloy

This comment has been minimized.

Copy link
Collaborator

@alloy alloy commented Mar 3, 2020

@todorone Yes, it’s available in the 0.62.0 RCs; please do try!

osdnk added a commit to osdnk/react-native that referenced this issue Mar 9, 2020
Summary:
0.62 Flipper Support items: facebook#27565
Made RNTester's Android Flipper implementation consistent with the template.
~~Added Flipper to the iOS build.~~
<img width="1259" alt="Screen Shot 2019-12-28 at 7 06 42 PM" src="https://user-images.githubusercontent.com/8675043/71551835-37290800-29a5-11ea-9eac-b119a69a68c1.png">

## Changelog

[Internal] [Added] - RNTester Android Fipper updates
Pull Request resolved: facebook#27631

Test Plan: Run RNTester and see if it connects to Flipper.

Reviewed By: rickhanlonii

Differential Revision: D19345093

Pulled By: passy

fbshipit-source-id: 6957c1ca3f4a5bb7f0e581c5daf8ddeac5d87eea
osdnk added a commit to osdnk/react-native that referenced this issue Mar 9, 2020
…7569)

Summary:
Issue: facebook#27565

initalizeFlipper should be set in template app by default.

## Changelog

[iOS] [Changed] - Added Flipper to template app
[Android] [Changed] - Added Flipper to template app
Pull Request resolved: facebook#27569

Test Plan:
Connect Flipper to the iOS application
Connect Flipper to the Android application

Reviewed By: passy

Differential Revision: D19344704

Pulled By: rickhanlonii

fbshipit-source-id: ca126fd2caab13751ddc2ce6d195bd0c644c704e
osdnk added a commit to osdnk/react-native that referenced this issue Mar 9, 2020
Summary:
Related to facebook#27426 & facebook#27565.

The wish to not include an empty Swift file to trigger the correct Xcode settings was voiced in facebook#27426 & facebook#27565.

## Changelog

[iOS] [Fixed] - Remove need for Swift file in the user’s project in order to use Flipper
Pull Request resolved: facebook#27922

Test Plan:
An application created with the template, like so:

```bash
react-native init --template=~/Code/React/react-native TestFlipper
```

…will successfully build, launch, and have Flipper connect.

Differential Revision: D19690592

Pulled By: cpojer

fbshipit-source-id: ee696e0d747d6338534b0c2d62029e64ece02cd3
osdnk added a commit to osdnk/react-native that referenced this issue Mar 9, 2020
Summary:
Addresses my feedback [here](facebook#27565 (comment)), [here](facebook#27565 (comment)), and [here](facebook#27565 (comment)).

## Changelog

[iOS] [Changed] - Updated Flipper iOS integration to be included by default in the `Debug` configuration
Pull Request resolved: facebook#28044

Test Plan:
Manually tested that a new application from this template still works and that the Flipper integration works.

<img width="912" alt="Screenshot 2020-02-13 at 02 09 42" src="https://user-images.githubusercontent.com/2320/74391951-eb6fd800-4e05-11ea-9fde-7e0eb42c1ec4.png">

Differential Revision: D19871482

Pulled By: TheSavior

fbshipit-source-id: a805808fdd0c2dfdfe47dd59ffee02c81f3fdfa7
@younes200

This comment has been minimized.

Copy link

@younes200 younes200 commented Apr 5, 2020

Hi,
Just to mention that ENABLE_BITCODE must be disable on debug, otherwise the build will fail complaining about the missing OpenSSL-Universal bitcode.

  - Flipper-Folly (2.1.1):
    - boost-for-react-native
    - CocoaLibEvent (~> 1.0)
    - Flipper-DoubleConversion
    - Flipper-Glog
    - OpenSSL-Universal (= 1.0.2.19)
@alloy

This comment has been minimized.

Copy link
Collaborator

@alloy alloy commented Apr 6, 2020

Going to close this, as this ship has set sail! :shipit:

@alloy alloy closed this Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.