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

[General] [Fixed] - Fixes react-native-codegen location within script_phases.sh #35763

Closed
wants to merge 1 commit into from

Conversation

peterpme
Copy link

@peterpme peterpme commented Jan 2, 2023

Summary

@byCedric did some amazing work to update the codegen location monorepo support. There was just 1 small issue which is in the name of the package: instead of @react-native/codegen it should be react-native-codegen

Changelog

[General] [Fixed] - Fixes react-native-codegen location within script_phases.sh from @react-native/codegen to react-native-codegen

Test Plan

Run eas build --platform ios within a monorepo like Backpack

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 2, 2023
@hoxyq
Copy link
Contributor

hoxyq commented Jan 2, 2023

Hey @peterpme, thanks for the contribution!

I see that this line was changed in #34804, after we renamed package react-native-codegen to @react-native/codegen. Although its location hasn't changed (it is still present inside packages/react-native-codegen), its also should be available as @react-native/codegen inside node_modules.

We probably changed these lines by a mistake. I don't have much of a context on how its used here, but does this revert fixes your problem, could you please validate that?

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,493,253 +0
android hermes armeabi-v7a 7,813,845 +0
android hermes x86 8,969,058 +0
android hermes x86_64 8,827,238 +0
android jsc arm64-v8a 9,678,873 +0
android jsc armeabi-v7a 8,413,102 +0
android jsc x86 9,743,267 +0
android jsc x86_64 10,220,680 +0

Base commit: 4ac4a5c
Branch: main

@pull-bot
Copy link

pull-bot commented Jan 2, 2023

PR build artifact for d9555e5 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@rshest
Copy link
Contributor

rshest commented Jan 2, 2023

This breaks the CI tests, please check e.g. here:

Error: Cannot find module 'react-native-codegen/package.json'

Copy link
Contributor

@rshest rshest left a comment

Choose a reason for hiding this comment

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

The CI builds are broken (see my comment above), looks it's because of this change.

if ! [ -d "$CODEGEN_CLI_PATH" ]; then
error "error: Could not determine @react-native/codegen location, using node module resolution. Try running 'yarn install' or 'npm install' in your project root."
error "error: Could not determine react-native-codegen location, using node module resolution. Try running 'yarn install' or 'npm install' in your project root."
Copy link
Contributor

Choose a reason for hiding this comment

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

Could make sense to make the react-native-codegen location a constant.

@peterpme
Copy link
Author

peterpme commented Jan 2, 2023

Hi @rshest & @hoxyq thank you! I think there's a version mismatch. Moving forward I believe this should be @react-native/codegen so I will close this out :)

@peterpme peterpme closed this Jan 2, 2023
@peterpme peterpme deleted the patch-2 branch January 2, 2023 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants