-
Notifications
You must be signed in to change notification settings - Fork 905
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: install pods on ios-commands with New Arch #2122
fix: install pods on ios-commands with New Arch #2122
Conversation
path.join(iosSourceDir, '/Pods/Pods.xcodeproj/project.pbxproj'), | ||
); | ||
|
||
return project.includes('-DRCT_NEW_ARCH_ENABLED=1'); |
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.
@cipolleschi is this fine if we use that to detect new arch?
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.
yes, this should be enough.
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.
@TMisiukiewicz please rebase and merge? :)
import {readFile} from 'fs-extra'; | ||
import path from 'path'; | ||
|
||
export default async function getArchitecture(iosSourceDir: string) { |
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.
We have the same logic in doctor
command:
cli/packages/cli-doctor/src/commands/info.ts
Lines 55 to 64 in 36d00ea
const project = await readFile( | |
path.join( | |
ctx.project.ios.sourceDir, | |
'/Pods/Pods.xcodeproj/project.pbxproj', | |
), | |
); | |
platforms.iOS.newArchEnabled = project.includes( | |
'-DRCT_NEW_ARCH_ENABLED=1', | |
); |
mind using this function also there - so we won't have in two places same code? 🙏
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.
it's already here:
const isNewArchitecture = await getArchitecture( |
ef7a66a
to
75a0e19
Compare
@szymonrybczak to resolve conflicts i moved |
throw new CLIError( | ||
`Something when wrong while installing CocoaPods. Please run ${chalk.bold( | ||
'pod install', | ||
)} manually`, | ||
); |
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.
Hm, I'm wondering should we add here info about flags that we added 🤔
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.
LGTM 🙌
Summary:
Coming from #2091. Currently, when installed pods with New Architecture manually, added new package and used
run-ios
, it installed pods with old Architecture again. This change checks if the New Architecture is enabled and installs it with/without a flag.Test Plan:
react-native-gesture-handler
run-ios
and verify it installed pods and ran the appios
folder and install pods with New Arch manually,RCT_NEW_ARCH_ENABLED=1 pod install
react-native-reanimated
run-ios
and verify pods are being installed with awith New Architecture
messagefabric: true
is visible in Metro consoleChecklist