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

Allow parallelizeBuildables on iOS builds to improve build time #1724

Merged
merged 93 commits into from
Jan 17, 2022

Conversation

kathaypacific
Copy link
Collaborator

@kathaypacific kathaypacific commented Jan 6, 2022

Description

The purpose of this PR is to enable us to use "dependency order" for the "build order". This is the default setting for xcode 13 and also since RN 0.62. The benefits include better build performance through more parallelisation and reduced dependency cycle errors.

Screenshot 2022-01-06 at 15 26 55

To build in this way, dependencies need to require the React-Core pod instead of React . For this reason, many dependencies needed to be updated. Where possible I have updated to the latest versions, some libs were deprecated so I have updated to the new recommended option.

Of note:

  • react-native-camera has been deprecated in favour of react-native-vision-camera, however react-native-vision-camera seems to serve a different purpose and doesn't have the QR barcode scanning functionalities we are needing. I opted to upgrade to the latest version of react-native-camera even though it is deprecated, as we were one major version behind and the latest version did have the podspec change we need for this piece of work.
  • react-native-exit-app also seems a little abandoned, i used a commit hash because Fix Xcode 12 compatibility. wumke/react-native-exit-app#45 (comment)

New patches:

  • react-native-keep-awake is deprecated in favour of expo-keep-awake, i opted to patch react-native-keep-awake since we don't have/want expo
  • react-native-sms/react-native-fast-crypto seem abandoned, i opted to patch instead of waiting for a new release with these 14 month old PRs here and here
  • clevertap-react-native doesn't have the podspec change we need in the latest release, so i have patched the existing version for now
  • react-native-securerandom seems abandoned, we are on the latest version.
  • react-native-svg we are on the latest version, there is a new pre-release that contains the fix we need but it has been on pre-release since 08/2021. there is an open issue to publish the pre-release so for now i have patched.
  • react-native-netinfo - after upgrading to the latest, the android CI started failing due to not correctly detecting the network connection status. rather than holding up the rest of this work, patch for now and investigate later. more context here

Other changes

Unlink react-native-geth and react-native-securerandom to resolve the below build warning. This was actually pretty straightforward, didn't cause any problems.

Screenshot 2022-01-07 at 14 23 48

Tested

Manually

How others should test

A regression to see that app functionality hasn't been affected. e.g. QR scanning, app data persistence when force closing/opening the app.

Backwards compatibility

Yes

kathaypacific and others added 30 commits December 28, 2021 11:31
…ner which is now deprecated and causing tests to fail
Base automatically changed from kathy/upgrade-RN-65 to main January 12, 2022 14:16
@@ -323,6 +323,7 @@ export default WalletConnect = () => {
await element(by.text('Connected Apps')).tap()
await element(by.text('Tap to Disconnect')).tap()
await element(by.text('Disconnect')).tap()
await element(by.id('BackChevron')).tap()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after apps are disconnected, the user is still on the Sessions screen but the next assertion for ConnectedApplications/value is on the Settings screen. because this assertion is in the afterAll hook, the tests failed without being captured by the test reports or test artifacts. i think for correctness the command to navigate back to the settings screen should exist...

(i also ran this test locally on the main branch, and it seems that the tests pass even though the app is still on the Sessions screen and the ConnectedApplications/value is not found :/ i'm not sure why it can pass, but the behaviour of disconnecting a connected app has not changed in this PR)

@kathaypacific kathaypacific marked this pull request as ready for review January 17, 2022 12:54
Copy link
Member

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! 💯 🚀

@kathaypacific kathaypacific added the automerge Have PR merge automatically when checks pass label Jan 17, 2022
@mergify mergify bot merged commit f1c65bd into main Jan 17, 2022
@mergify mergify bot deleted the kathy/ios-parallelize-buildables branch January 17, 2022 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants