-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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 hardcoded path to codegen cli for monorepos #35430
Conversation
Hi @byCedric, thank you so much for the contribution. Could you log into https://app.circleci.com, please? |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
No problem @cipolleschi :) I just authenticated, but it might be related to me creating this PR from a fork? It also seems that I can't manually trigger a restart from CircleCI. Either way, there is one thing that might be good to add to this fix. Technically, for isolated modules, this fix is not 100% correct. In isolation mode, the current directory where But this is sufficient for the current package managers :) |
/rebase |
6ec44f5
to
189b5cb
Compare
Base commit: e11cfe9 |
Base commit: 9517320 |
PR build artifact for 189b5cb is ready. |
PR build artifact for 189b5cb is ready. |
can't this be addressed in this PR? I am not aware of any reasons why the package.json of codegen can't have the main field set |
I think it's a side-effect worth mentioning but not necessarily required to fix. We'd probably always want to resolve the If you have an entry point like But maybe just adding a |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Thank you @byCedric for the PR and thanks to everyone else for reviewing it :) Happy Thanksgiving! |
This pull request was successfully merged by @byCedric in 4a4ccee. When will my fix make it into a release? | Upcoming Releases |
Summary: Fixes #35429 This fix is fairly straightforward. Instead of hardcoding two separate paths for this repo or a simple user's project, we ask Node to resolve `react-native-codegen`. Because `react-native-codegen` is moved [into the `/packages/*` folder](https://github.com/facebook/react-native/blob/main/package.json#L104), it's part of a workspace in this repository. That means that this fix works not only for monorepos in general but also resolves the right path within the react native repository. You can test this out yourself when running this command in either the root, or any other workspace in this repository: ```bash node --print "require('path').dirname(require.resolve('react-native-codegen/package.json'))" ``` I've tested this on Node 16 and 18, and seems to work nicely. Note that you _**have to specify `react-native-codegen/package.json`**_. That's because the `react-native-codegen` package itself is technically invalid; it's missing an entry point within the package (no `main`). When running `node --print "require.resolve('react-native-codegen')"` Node can't resolve the main entry point, and will fail during this process. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [iOS] [Fixed] - Fix incorrect codegen CLI paths in monorepo projects Pull Request resolved: #35430 Test Plan: See PR #35429 Reviewed By: cortinico Differential Revision: D41475878 Pulled By: cipolleschi fbshipit-source-id: f0c362b64cf9c3543a3a031d7eaf302c1314e3f0
Summary: Fixes facebook#35429 This fix is fairly straightforward. Instead of hardcoding two separate paths for this repo or a simple user's project, we ask Node to resolve `react-native-codegen`. Because `react-native-codegen` is moved [into the `/packages/*` folder](https://github.com/facebook/react-native/blob/main/package.json#L104), it's part of a workspace in this repository. That means that this fix works not only for monorepos in general but also resolves the right path within the react native repository. You can test this out yourself when running this command in either the root, or any other workspace in this repository: ```bash node --print "require('path').dirname(require.resolve('react-native-codegen/package.json'))" ``` I've tested this on Node 16 and 18, and seems to work nicely. Note that you _**have to specify `react-native-codegen/package.json`**_. That's because the `react-native-codegen` package itself is technically invalid; it's missing an entry point within the package (no `main`). When running `node --print "require.resolve('react-native-codegen')"` Node can't resolve the main entry point, and will fail during this process. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [iOS] [Fixed] - Fix incorrect codegen CLI paths in monorepo projects Pull Request resolved: facebook#35430 Test Plan: See PR facebook#35429 Reviewed By: cortinico Differential Revision: D41475878 Pulled By: cipolleschi fbshipit-source-id: f0c362b64cf9c3543a3a031d7eaf302c1314e3f0
Summary
Fixes #35429
This fix is fairly straightforward. Instead of hardcoding two separate paths for this repo or a simple user's project, we ask Node to resolve
react-native-codegen
.Because
react-native-codegen
is moved into the/packages/*
folder, it's part of a workspace in this repository. That means that this fix works not only for monorepos in general but also resolves the right path within the react native repository.You can test this out yourself when running this command in either the root, or any other workspace in this repository:
node --print "require('path').dirname(require.resolve('react-native-codegen/package.json'))"
I've tested this on Node 16 and 18, and seems to work nicely. Note that you have to specify
react-native-codegen/package.json
. That's because thereact-native-codegen
package itself is technically invalid; it's missing an entry point within the package (nomain
). When runningnode --print "require.resolve('react-native-codegen')"
Node can't resolve the main entry point, and will fail during this process.Changelog
[iOS] [Fixed] - Fix incorrect codegen CLI paths in monorepo projects
Test Plan
See PR #35429