bamlab / react-native-flipper-performance-monitor

An attempt to have a lighthouse for React Native. Flipper plugin to show a graph of the React Native performance monitor
MIT License
586 stars 17 forks source link

chore: only use Flipper in DEBUG builds #64

Closed kirillzyusko closed 2 years ago

kirillzyusko commented 2 years ago

Similarly to https://github.com/facebook/flipper/commit/dd111076c9f6bcfbbc0b0be454b16395dab8fcaf

Just found out that in release mode it still compiles this Pod and I think it's not desired behaviour. If it's desired, then please correct me 😄

kirillzyusko commented 2 years ago

I think this PR attempts to solve a similar problem from https://github.com/bamlab/react-native-flipper-performance-monitor/pull/62 PR

Honestly, I'm not sure which approach is better... The essence of your PR is that you look at whether the flipper is available, while this PR simply turns off the plugin in a release build (this is the default behaviour of the flipper itself).

Your approach seems a bit more generic to me, though I can't imagine a situation where flipper might not be available (only if the person installs this package and doesn't have an installed flipper?). And this approach simply reflects the logic that is used in the original flipper package 🙂

Almouro commented 2 years ago

Hi @kirillzyusko, thanks for the pull request! You're right, this reflects what is done in the original flipper package and indeed, having the code for this package compiled in production is not intented. So your PR definitely solves that!

I went for https://github.com/bamlab/react-native-flipper-performance-monitor/pull/62 as this also helps in the case where you have Flipper installed on Android but not on iOS (if you have use_frameworks enabled for instance). So you would be in DEBUG but still not have Flipper installed.

The issue is this package is both a Flipper plugin and a RN native module (it's kind of a hack though because I wanted to have auto linking 😅), so by default it will install automatically on both platforms.

kirillzyusko commented 2 years ago

@Almouro aha, I got it. Then I think you can close this PR and go with https://github.com/bamlab/react-native-flipper-performance-monitor/pull/62 👍

Almouro commented 2 years ago

👌 Closing in favor of #62 then