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

feat: RFC-0759: command init for iOS → @react-native/core-cli-utils #2332

Draft
wants to merge 1 commit into
base: rfc-0759-changes
Choose a base branch
from

Conversation

blakef
Copy link
Contributor

@blakef blakef commented Mar 15, 2024

Summary:

Use the core library for iOS init actions.

Warning

This needs to wait for nightlies to land with the updated TypeScript definitions, expect CI to fail until then.

Test Plan:

Checklist

  • Documentation is up to date to reflect these changes.
  • Follows commit message convention described in CONTRIBUTING.md

@@ -8,6 +8,7 @@
},
"dependencies": {
"@react-native-community/cli-tools": "14.0.0-alpha.0",
"@react-native/core-cli-utils": "nightly",
Copy link
Member

Choose a reason for hiding this comment

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

Same as for Android PR, this needs to be updated once we have the first RC out

Copy link
Member

Choose a reason for hiding this comment

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

@cortinico could you please add me, @szymonrybczak and @TMisiukiewicz as Codeowners for @react-native/core-cli-utils and other former CLI folders? This would make it easier for us to track changes and participate in discussions and development of these

Copy link
Member

Choose a reason for hiding this comment

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

We don't use CODEOWNERS for react-native sadly

shouldHandleRepoUpdate: false,
newArchEnabled: options?.newArchEnabled,
Copy link
Member

Choose a reason for hiding this comment

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

Why are you removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ...options: RunPodInstallOptions includes whether newArchEnabled has been configured on / off.

Copy link
Member

Choose a reason for hiding this comment

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

Great thanks for the clarification

@thymikee
Copy link
Member

Trying to get my head around the logic of installing pods under various conditions. Is this a temporary step? Because it moves some of the logic to the core, but not most of it.

Other than that and lack of types (which we can ts-expect-error and ship later) it seems to work when linked locally

@blakef
Copy link
Contributor Author

blakef commented Apr 2, 2024

Trying to get my head around the logic of installing pods under various conditions. Is this a temporary step? Because it moves some of the logic to the core, but not most of it.

Other than that and lack of types (which we can ts-expect-error and ship later) it seems to work when linked locally

Thanks for taking a look at this @thymikee. Appreciate the feedback. What sort of things would you have expected to move into core that aren't there?

Both flow and TypeScript types ship with the @react-native/core-cli-utils package.

@thymikee
Copy link
Member

Since we have TS typings, can we rebase and hopefully get it merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants