-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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: correctly forward dependency on @react-native-community/cli-server-api
#49325
base: main
Are you sure you want to change the base?
Conversation
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@@ -98,6 +98,7 @@ | |||
"featureflags": "node ./scripts/featureflags/index.js" | |||
}, | |||
"peerDependencies": { | |||
"@react-native-community/cli-server-api": "*", |
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.
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 still need it here for it to be hoisted correctly. But we can make it optional and make it a direct dependency in the template.
Edit: I meant to submit a PR to the template but got distracted. Will try to get to it later tonight.
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 still need it here for it to be hoisted correctly
Why do we need it here specifically?
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.
Basically, we declare that @react-native/community-cli-plugin
depends on cli-server-api
but that consumers need to satisfy this requirement. The only consumer of @react-native/community-cli-plugin
is react-native
, but you don't want react-native
to depend on CLI any longer. So react-native
needs to "forward" the dependency to its consumers, hence redeclare peer dependency. Without this, package managers won't correctly link the package.
However, since react-native
has cli-server-api
as peer dependency, it's probably fine to mark it as optional and add it in the template as a direct dependency to satisfy the requirement.
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.
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.
but you don't want react-native to depend on CLI any longer.
I think there is some cleanup to make sure react-native
doesn't need to depend on @react-native/community-cli-plugin
at all so this change wont' be required at all
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.
I think there is some cleanup to make sure
react-native
doesn't need to depend on@react-native/community-cli-plugin
at all so this change wont' be required at all
Yes, that would be the proper solution in the long run.
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.
For the time being, can't we just have:
- Remove the
peerDependency
here betweenreact-native
and@react-native-community/cli-server-api
(so amend this line) - Add the dependency on the template as per fix:
@react-native/community-cli-plugin
depends on@react-native-community/cli-server-api
react-native-community/template#105
?
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.
I've replied to this offline but I'll post it here as well for transparency: The short answer is, if you do it this way, package managers won't put @react-native-community/cli-server-api
in a place where @react-native/community-cli-plugin
will be able to find it.
Longer answer: See #49325 (comment)
Summary:
@react-native/community-cli-plugin
depends on@react-native-community/cli-server-api
but incorrectly declares it as optional. When the dependency is not found, thebundle
andstart
commands are not registered. This change removes the optional flag and forwards the dependency to consumers ofreact-native
.Resolves #47309
Changelog:
[GENERAL] [FIXED] - Fix
@react-native-community/cli-server-api
not being found under certain hoisting conditionsTest Plan:
See #47309 for repro steps.