fix(build): fixes React-RCTText build with RN 0.69.0 #34064
Conversation
|
Hi @ph4r05! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
facebook-github-bot
added
CLA Signed
Base commit: afa5df1 |
|
I can confirm it fixes things for me, but I am not sure why the I feel like this PR is a workaround the real issue ( If we go with this fix, then the root problem is still present and other packages will need to apply the same fix as the root cause is still here, for example |
Base commit: 7d42106 |
|
@sshic has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Thanks for sending this over @ph4r05
We're going to merge this as it was confirmed as valid by @renchap here #33976 (comment)
I'll defer to @cipolleschi for a follow-up on the iOS side if we need one.
Also as an heads up, this is currently failing to build RN Tester internally with: |
|
Hi @ph4r05 and @renchap. Thanks for opening the PR. Could you pleas provide a sample repo with a repro of the issue? I just created an app with 0.69 and tried to build with both the legacy and the New Architecture and it builds without issues and without applying the patch. |
Hi @cipolleschi, Sorry for not yet providing a sample reproducing the issue, I am in the final steps of releasing an app and did not had time to investigate this as properly as I wanted. This is most probably only occurring when a dependency is used. In the linked issue (#33976) we noticed that |
|
@renchap I tried to add the Then I tried both to install the pods with the new and the legacy architectures. In both cases i could see the pod in the project and the pod install script reporting this line: In both case, after a clean, the app was building and running. I don't think we need this fix at all. My best guess is that a step was missing when migrating the app. |
|
@cipolleschi I am working on the PoC app, I will have it today. I think it has also something to do with Expo modules. |
|
@cipolleschi this is a minimal PoC that behaves as described in the issue: https://github.com/ph4r05/poc-rn-34064 Env: |
|
@renchap pls which react-native version do you use in the project? |
|
RN 0.69.0 |
|
@ph4r05 RN 0.69.0. But I am also using expo (for some Expo modules, not the whole thing), so this could come from Expo indeed. |
|
I'm trying out the PoC
I then tried to isolate which change may generate that:
So, there is something in the From this investigation, it looks like that it's Expo that is not supporting 0.69 yet and I don't think we have to actually merge this PR into React Native. Instead, we should wait for Expo to properly support this new version. @Kudo what do you think? |
|
for expo to integrate React-Core from swift code, we have some patches for React-Core. sometimes the clang compiler requires the imports to be in correct order. otherwise, it will import to wrong headers and have build errors. my fix toward the problem is like that, maybe it's lower risk. could you check whether it works for you and makes sense to you? diff --git a/node_modules/react-native/React/Views/RCTShadowView.h b/node_modules/react-native/React/Views/RCTShadowView.h
index 428d2eb..226672f 100644
--- a/node_modules/react-native/React/Views/RCTShadowView.h
+++ b/node_modules/react-native/React/Views/RCTShadowView.h
@@ -8,6 +8,8 @@
#import <UIKit/UIKit.h>
#import <React/RCTComponent.h>
+// Keeps RCTConvert.h here before yoga for clang module to generate correct header imports.
+#import <React/RCTConvert.h>
#import <React/RCTLayout.h>
#import <React/RCTRootView.h>
#import <yoga/Yoga.h> |
|
@Kudo this worked! Shall I change this PR so it reflects this change? |
|
@ph4r05 sure, please update the pr if the change makes sense to you. |
ph4r05
force-pushed
the
pr/fix-react-rcttext
branch
from
d7f3a27
to
ccf210a
Compare
|
done! Great catch with the order of imports @Kudo ! |
|
thanks everyone here to fix the issue together 🔥 |
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
# Why for sdk 46 close ENG-5353 # How - update packages - react -> 18.0.0 - react-native -> 0.69.0 - react-dom -> 18.0.0 - react-native-web -> ~0.18.1 - react-test-renderer -> ~18.0.0 - @types/react -> ~18.0.14 - @types/react-native -> ~0.69.1 - early patch react-native for 0.69.1 fixes - facebook/react-native@43f831b - facebook/react-native@c2088e1 - facebook/react-native@f97c6a5 - facebook/react-native@79baca6 - facebook/react-native#34064 (comment) - migrate `expo-template-bare-minimum`, `bare-expo`, `bare-sandbox` to 0.69. reference from https://react-native-community.github.io/upgrade-helper/?from=0.68.1&to=0.69.0 - also remove the `hermesCommand` because 0.69 uses the builtin hermes-engine - `expo-av`, `expo-modules-core`, `expo-gl-cpp`: fix android cpp building errors, because 0.69 ships both release and debug aar - `expo-dev-client`: fix android build error from RedBox package rename - `html-elements`: update RNWView where react-native-web removed [the `css` import](necolas/react-native-web@b27c9820) - `expo`: move Expo.podspec out from ios, because newer [rn-cli removed the `podspecPath` and `project` support](react-native-community/cli@25eec7c#diff-0dddbcedebb33032fcac5991f3dcdfa44157e6ae87afcf3dabcd240a0db09832L58). - [ ] disable dev-client because the vendored reanimated in dev menu doesn't support 0.69 yet, e.g. `folly_json -> folly_runtime` - update reanimated and gesture-handler to latest for 0.69 support - `jest-expo`: update to [fix the deprecated react-native-web jest preset](necolas/react-native-web@9b0c119) - `NCL`, `home`, patch `react-native-svg`: fix react 18 that [`children` should be explicitly added](https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#updates-to-typescript-definitions) - `@expo/cli` fix react-native-web and react-dom version checks # Test Plan - android bare-expo smoke test - ios bare-expo smoke test - ci passed - updates e2e is broken because in the flow the react-native doesn't include 0.69.1 fixes.
|
This pull request was successfully merged by @ph4r05 in 4ea38e1. When will my fix make it into a release? | Upcoming Releases |
Summary: Fixes iOS build for React-RCTText with RN 0.69.0, fixes #33976 PR contains changes from #33976 (comment) PoC repo: https://github.com/ph4r05/poc-rn-34064 Related issues: - expo/expo#16283 - #33815 - #33976 ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://github.com/facebook/react-native/wiki/Changelog --> [iOS] [Fixed] - Fix build for React-RCTText Pull Request resolved: #34064 Reviewed By: cortinico Differential Revision: D37420163 Pulled By: cipolleschi fbshipit-source-id: 68a831bce9f449348d13e040a1ba12726a66667d
Summary
Fixes iOS build for React-RCTText with RN 0.69.0, fixes #33976
PR contains changes from #33976 (comment)
PoC repo: https://github.com/ph4r05/poc-rn-34064
Related issues:
Changelog
[iOS] [Fixed] - Fix build for React-RCTText
Test Plan