-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Exclude react-native-flipper
when NO_FLIPPER=1
to prevent iOS build fail
#35686
Conversation
Base commit: e625616 |
Base commit: e625616 |
PR build artifact for 4ebba36 is ready. |
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.
Hi @retyui, thank you for the PR.
Can I ask you to expand on the use case, please?
In the repro, you are adding a library that depends on Flipper (react-native-flipper
) and then you are NOT installing Flipper, by passing NO_FLIPPER=1
. I think it is correct that the app doesn't build with that setup.
If you need to use react-native-flipper
, you have to also install Flipper in your app (the default setup, or, in other words NO_FLIPPER=0
).
On a side note, the react-native.config.js
is a configuration file specific for each project. It should not be responsibility of React Native to preconfigure it. Every app should be able to add it and customise it as they prefer. We don't want to prefill it with some default values: users may get confused why they need them and/or they may need to change it and React Native should not depend on some specific settings there.
@cipolleschi that's a popular optimization for developers to turn off Flipper on CI to speed up iOS builds, Previously it was a developer responsibility to add an But starting from So, to be more developer friendly we can add a comment about |
And that's fine, but I if we add a reference to What do you think? |
I partially agree with proposed solution. But yeah, it should not be in template config as long as there is no The "why" that led me here was a use of flipper react-navigation plugin, which is dependent on react-native-flipper package. |
4ebba36
to
d2bd5e9
Compare
PR build artifact for d2bd5e9 is ready. |
Thanks for updating the PR. I'll proceed importing it and trying to land it. Meanwhile, I also created this PR on the website: facebook/react-native-website#3503 What do you think? |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
I agree that having a proposed solution in form of a comment inside |
We can show warn on # @react-native-community/cli-platform-ios/native_modules.rb
packages = config["dependencies"]
found_pods = []
if (packages['react-native-flipper'] && ENV["NO_FLIPPER"] == "1") # <- simple check
Pod::UI.warn " warning text "
end |
@retyui Yes, warning is better than a hard error 👍 |
Ok, then I close this PR and will open PR in |
@retyui We can probably also do it here https://github.com/facebook/react-native/blob/main/scripts/react_native_pods.rb#L211
@cipolleschi What do you think? |
I personally don't think that the solution. I don't think that it is React Native responsibility to handle this problem in code. React Native exposes some dependencies, but we can't control what happens outside them. What we can do is to point users to the proper documentation (e.g.: facebook/react-native-website#3503). |
Since |
We can do that, but where would you put the caution note? In the website PR there is the explicit example. What we should actually do is to update the README.md of the |
I don't believe updating the README.md of As usually developers install this modules as dev-tools required ( as that dccs. don't referring to the original Installation sections of As is typical, developers install this module when certain dev-tools are needed (because that docs. don't refer to the original "Installation" sections of See: |
Thanks for the context. I have to think more about a proper solution, then. |
@cipolleschi merged this pull request in f47b5b8. |
…ld fail (#35686) Summary: - Flipper issue: facebook/flipper#3995 (comment) iOS build fail with an error: ```sh node_modules/react-native-flipper/ios/FlipperReactNativeJavaScriptPlugin.h:9:9: 'FlipperKit/FlipperConnection.h' file not found #import <FlipperKit/FlipperConnection.h> ``` ## Changelog [IOS] [FIXED] - Exclude `react-native-flipper` when `NO_FLIPPER=1` to prevent iOS build fail Pull Request resolved: #35686 Test Plan: ```sh npx react-native init RN0710RC5 --version 0.71.0-rc.5 cd RN0710RC5 yarn add react-native-flipper NO_FLIPPER=1 pod install --project-directory=ios yarn ios # will fail ``` Reviewed By: rshest Differential Revision: D42368444 Pulled By: cipolleschi fbshipit-source-id: a8614ccadb98970ebae15d8743136fa60ead318c
…ld fail (facebook#35686) Summary: - Flipper issue: facebook/flipper#3995 (comment) iOS build fail with an error: ```sh node_modules/react-native-flipper/ios/FlipperReactNativeJavaScriptPlugin.h:9:9: 'FlipperKit/FlipperConnection.h' file not found #import <FlipperKit/FlipperConnection.h> ``` ## Changelog [IOS] [FIXED] - Exclude `react-native-flipper` when `NO_FLIPPER=1` to prevent iOS build fail Pull Request resolved: facebook#35686 Test Plan: ```sh npx react-native init RN0710RC5 --version 0.71.0-rc.5 cd RN0710RC5 yarn add react-native-flipper NO_FLIPPER=1 pod install --project-directory=ios yarn ios # will fail ``` Reviewed By: rshest Differential Revision: D42368444 Pulled By: cipolleschi fbshipit-source-id: a8614ccadb98970ebae15d8743136fa60ead318c
Summary
iOS build fail with an error:
Changelog
[IOS] [FIXED] - Exclude
react-native-flipper
whenNO_FLIPPER=1
to prevent iOS build failTest Plan