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

fix: react native 0.73.x build failing when new arch is enabled on iOS #304

Merged
merged 2 commits into from
Dec 23, 2023

Conversation

Mhp23
Copy link
Contributor

@Mhp23 Mhp23 commented Dec 21, 2023

📜 Description

Updating Pod to add Yoga style to HEADER_SEARCH_PATHS for new architecture

💡 Motivation and Context

For react native (0.73.X) build failing when new architecture is enabled on iOS.

📢 Changelog

Fix: yoga/style/Style.h not found when new arch is enabled on iOS

iOS

  • yoga/style/Style.h header has added to HEADER_SEARCH_PATHS on iOS Pods.

🤔 How Has This Been Tested?

📸 Screenshots (if appropriate):

keyboard-controller

📝 Checklist

  • CI successfully passed

@kirillzyusko
Copy link
Owner

@Mhp23 thank you for your PR!

@Mhp23 is it kind of a change for all libraries? I'm not using YogaStylableProps anywhere directly, so just curious if other libraries have this fix in place? 🤔

Also will it be compatible with older RN versions? (though I think CI should tell me about it, since I'm still building on RN 0.72)

@Mhp23
Copy link
Contributor Author

Mhp23 commented Dec 21, 2023

@kirillzyusko, you're welcome.
I'm not sure if it's required for all libraries, but I recently encountered a problem with this particular library, as well as react-native-webview. It seems to be happening with some libraries on RN 0.73.X version (I haven't tested it with older versions).

@kirillzyusko
Copy link
Owner

@Mhp23 I have several questions :)

  1. Did you try to add something similar to chore(iOS): Use install_modules_dependencies in Podspec lottie-react-native/lottie-react-native#1139? Maybe it would resolve the problem?
  2. Podfile.lock in FabricExample is not changed after pod install? I tested locally and hash gets changed
  3. May I kindly ask you to rebase your changes to latest main? I've added missing paths, so CI will check a compatibility of this change with currently supported RN version (RN 72)

kirillzyusko added a commit that referenced this pull request Dec 22, 2023
## 📜 Description

Build iOS projects when main .podspec file is changed.

## 💡 Motivation and Context

Was discovered in
#304

## 📢 Changelog

### CI
- added `'react-native-keyboard-controller.podspec'` to `paths`;

## 🤔 How Has This Been Tested?

Tested on CI.

## 📝 Checklist

- [x] CI successfully passed
@Mhp23
Copy link
Contributor Author

Mhp23 commented Dec 23, 2023

@kirillzyusko

  1. I have checked the mentioned pull request for lottie-react-native, and I attempted to use install_modules_dependencies in a similar manner, but it did not provide any help and the problem remained unresolved.

  2. Yes, it is, the hash identifier of react-native-keyboard-controller has been changed.

  3. I have used the latest main branch (with this commit: b4ca627), and I have applied the changes to the Pod and FabricExample builds successfully. However, due to some network issues, I tried multiple times and I am currently unable to pass the build CI. As you know, the CI build process is time-consuming (I run the CI locally), and I am unable to perform it at the moment. If possible, check it for further assurance.

As mentioned before, this issue may be related to React Native and could be resolved in the future, or with specific configurations, such as adding or removing certain elements and using install_modules_dependencies may also help solve the problem. I hope this information is helpful to you.

@kirillzyusko
Copy link
Owner

kirillzyusko commented Dec 23, 2023

@Mhp23 thank you for the detailed answer! I did some experiments with install_modules_dependencies in #308 and it works for me in empty project. May I kindly ask you to check it and confirm whether it fixes a problem in your project as well or not?

P. S. but seems to fail on RN 0.72 😔

@Mhp23
Copy link
Contributor Author

Mhp23 commented Dec 23, 2023

@kirillzyusko No problem,
Yes, it's working on my RN project too.

@kirillzyusko
Copy link
Owner

@Mhp23 it feels like the fix with install_modules_dependencies is a little bit complex (we need to use USE_FRAMEWORKS=dynamic flag for pods installation) and now I don't feel a confidence in all these actions.

So as a temporary solution I'm happy to go with this fix (most likely I'll publish a new version under 1.10.0 in next few days) and after that I'll revise a solution with install_modules_dependencies.

Thank you for your contribution!

@kirillzyusko kirillzyusko merged commit a425e7c into kirillzyusko:main Dec 23, 2023
7 of 8 checks passed
@kirillzyusko kirillzyusko self-assigned this Dec 27, 2023
@kirillzyusko kirillzyusko added 🍎 iOS iOS specific 🏭 fabric Changes specific to new (fabric/jsi) architecture labels Dec 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏭 fabric Changes specific to new (fabric/jsi) architecture 🍎 iOS iOS specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants