breaking: Fix ability to override commands via config #1999

huntie · 2023-07-05T10:22:34Z

Summary

I noticed a non-obvious bug when attempting to use react-native.config.js to override commands (i.e. the plugin system) over the CLI's built-in defaults.

While the below printout shows the project-configured commands being successfully read after the internal commands, these are never applied — due to a bug in all versions of the commander library where commands are stored as an array structure where the first registered command with a given name is used (and silently failing upon re-registering a command later 😐).

Changelog:
[Breaking] Custom commands in react-native.config.js files in libraries or projects will now override built-in CLI commands if defined with the same command name

Test Plan

Set up more console.logs
Build
yarn link to local React Native project with extended config ⬇️

module.exports = {
  commands: [
    ...ios.commands,
    ...android.commands,
    bundleCommand,
    ramBundleCommand,
    startCommand,
  ],

✅ Command implementation for react-native start can be successfully overridden

cc @tido64 for any potential downstream impact on rnx-kit

packages/cli/src/index.ts

huntie · 2023-07-17T18:54:02Z

@tido64 @thymikee Bump :)

tido64

You have my symbolic vote. 😄

szymonrybczak

🙌

Summary: Pull Request resolved: facebook#38795 ## Context RFC: Decoupling Flipper from React Native core: react-native-community/discussions-and-proposals#641 ## Changes Inits new package `react-native/community-cli-plugin`. This migrates [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) into the React Native repo, to enable faster iteration by the React Native core team. Specifically: - This package contains several `metro` dependencies, which when removed from CLI will no longer require us to ship new CLI releases to get Metro patches and features to users. - This package contains the `start`, `bundle`, and `ram-bundle` commands (central to the React Native development experience), for which we have incoming debugging-related changes. - This package now **only** exports commands to be attached via a RN CLI plugin. With this move, we're aiming to **internalise** the default implementations of these dev commands within React Native — other RN CLI plugins can continue to override these, but must do so wholesale. (See also the recent fix for this: react-native-community/cli#1999.) In V15: - (Microsoft feedback) Re-export `unstable_buildBundleWithConfig`, marking as unstable. This gives us a time buffer to consider how we repackage this functionality in future. The package source has been converted from TypeScript to Flow, with a number of new `flow-typed/` defs added to meet type coverage requirements. ## To dos - For now, we aren't removing the existing [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) source — until later PRs consolidate this move by changing dependencies in the `react-native` package. - **Exported API is reduced!**: I'm working with szymonrybczak to decouple references from RN CLI packages react-native-community/cli#2021. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D46801501 fbshipit-source-id: f704ff908eec1e7c2dd5aa71ee48204e5bd0a571

Summary: Pull Request resolved: facebook#38795 ## Context RFC: Decoupling Flipper from React Native core: react-native-community/discussions-and-proposals#641 ## Changes Inits new package `react-native/community-cli-plugin`. This migrates [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) into the React Native repo, to enable faster iteration by the React Native core team. Specifically: - This package contains several `metro` dependencies, which when removed from CLI will no longer require us to ship new CLI releases to get Metro patches and features to users. - This package contains the `start`, `bundle`, and `ram-bundle` commands (central to the React Native development experience), for which we have incoming debugging-related changes. - This package now **only** exports commands to be attached via a RN CLI plugin. With this move, we're aiming to **internalise** the default implementations of these dev commands within React Native — other RN CLI plugins can continue to override these, but must do so wholesale. (See also the recent fix for this: react-native-community/cli#1999.) In V15: - (Microsoft feedback) Re-export `unstable_buildBundleWithConfig`, marking as unstable. This gives us a time buffer to consider how we repackage this functionality in future. The package source has been converted from TypeScript to Flow, with a number of new `flow-typed/` defs added to meet type coverage requirements. ## To dos - For now, we aren't removing the existing [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) source — until later PRs consolidate this move by changing dependencies in the `react-native` package. - **Exported API is reduced!**: I'm working with szymonrybczak to decouple references from RN CLI packages react-native-community/cli#2021. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D46801501 fbshipit-source-id: ebd78383317b9b9f806d22a1e3d0e375b3b0f0aa

Summary: Pull Request resolved: facebook#38795 ## Context RFC: Decoupling Flipper from React Native core: react-native-community/discussions-and-proposals#641 ## Changes Inits new package `react-native/community-cli-plugin`. This migrates [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) into the React Native repo, to enable faster iteration by the React Native core team. Specifically: - This package contains several `metro` dependencies, which when removed from CLI will no longer require us to ship new CLI releases to get Metro patches and features to users. - This package contains the `start`, `bundle`, and `ram-bundle` commands (central to the React Native development experience), for which we have incoming debugging-related changes. - This package now **only** exports commands to be attached via a RN CLI plugin. With this move, we're aiming to **internalise** the default implementations of these dev commands within React Native — other RN CLI plugins can continue to override these, but must do so wholesale. (See also the recent fix for this: react-native-community/cli#1999.) In V15: - (Microsoft feedback) Re-export `unstable_buildBundleWithConfig`, marking as unstable. This gives us a time buffer to consider how we repackage this functionality in future. The package source has been converted from TypeScript to Flow, with a number of new `flow-typed/` defs added to meet type coverage requirements. ## To dos - For now, we aren't removing the existing [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) source — until later PRs consolidate this move by changing dependencies in the `react-native` package. - **Exported API is reduced!**: I'm working with szymonrybczak to decouple references from RN CLI packages react-native-community/cli#2021. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D46801501 fbshipit-source-id: 043e2aa86c23fbe8393368cfbdae2a0da3feda8e

Summary: Pull Request resolved: facebook#38795 ## Context RFC: Decoupling Flipper from React Native core: react-native-community/discussions-and-proposals#641 ## Changes Inits new package `react-native/community-cli-plugin`. This migrates [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) into the React Native repo, to enable faster iteration by the React Native core team. Specifically: - This package contains several `metro` dependencies, which when removed from CLI will no longer require us to ship new CLI releases to get Metro patches and features to users. - This package contains the `start`, `bundle`, and `ram-bundle` commands (central to the React Native development experience), for which we have incoming debugging-related changes. - This package now **only** exports commands to be attached via a RN CLI plugin. With this move, we're aiming to **internalise** the default implementations of these dev commands within React Native — other RN CLI plugins can continue to override these, but must do so wholesale. (See also the recent fix for this: react-native-community/cli#1999.) In V15: - (Microsoft feedback) Re-export `unstable_buildBundleWithConfig`, marking as unstable. This gives us a time buffer to consider how we repackage this functionality in future. The package source has been converted from TypeScript to Flow, with a number of new `flow-typed/` defs added to meet type coverage requirements. ## To dos - For now, we aren't removing the existing [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) source — until later PRs consolidate this move by changing dependencies in the `react-native` package. - **Exported API is reduced!**: I'm working with szymonrybczak to decouple references from RN CLI packages react-native-community/cli#2021. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D46801501 fbshipit-source-id: 0697f2d9af49034cbf39245bea59e9fe30c14dd0

Summary: Pull Request resolved: facebook#38795 ## Context RFC: Decoupling Flipper from React Native core: react-native-community/discussions-and-proposals#641 ## Changes Inits new package `react-native/community-cli-plugin`. This migrates [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) into the React Native repo, to enable faster iteration by the React Native core team. Specifically: - This package contains several `metro` dependencies, which when removed from CLI will no longer require us to ship new CLI releases to get Metro patches and features to users. - This package contains the `start`, `bundle`, and `ram-bundle` commands (central to the React Native development experience), for which we have incoming debugging-related changes. - This package now **only** exports commands to be attached via a RN CLI plugin. With this move, we're aiming to **internalise** the default implementations of these dev commands within React Native — other RN CLI plugins can continue to override these, but must do so wholesale. (See also the recent fix for this: react-native-community/cli#1999.) In V15: - (Microsoft feedback) Re-export `unstable_buildBundleWithConfig`, marking as unstable. This gives us a time buffer to consider how we repackage this functionality in future. The package source has been converted from TypeScript to Flow, with a number of new `flow-typed/` defs added to meet type coverage requirements. ## To dos - For now, we aren't removing the existing [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) source — until later PRs consolidate this move by changing dependencies in the `react-native` package. - **Exported API is reduced!**: I'm working with szymonrybczak to decouple references from RN CLI packages react-native-community/cli#2021. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D46801501 fbshipit-source-id: a21e7b563b0c9e0348034299fe25be7c5973e921

Summary: Pull Request resolved: facebook#38795 ## Context RFC: Decoupling Flipper from React Native core: react-native-community/discussions-and-proposals#641 ## Changes Inits new package `react-native/community-cli-plugin`. This migrates [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) into the React Native repo, to enable faster iteration by the React Native core team. Specifically: - This package contains several `metro` dependencies, which when removed from CLI will no longer require us to ship new CLI releases to get Metro patches and features to users. - This package contains the `start`, `bundle`, and `ram-bundle` commands (central to the React Native development experience), for which we have incoming debugging-related changes. - This package now **only** exports commands to be attached via a RN CLI plugin. With this move, we're aiming to **internalise** the default implementations of these dev commands within React Native — other RN CLI plugins can continue to override these, but must do so wholesale. (See also the recent fix for this: react-native-community/cli#1999.) In V15: - (Microsoft feedback) Re-export `unstable_buildBundleWithConfig`, marking as unstable. This gives us a time buffer to consider how we repackage this functionality in future. The package source has been converted from TypeScript to Flow, with a number of new `flow-typed/` defs added to meet type coverage requirements. ## To dos - For now, we aren't removing the existing [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) source — until later PRs consolidate this move by changing dependencies in the `react-native` package. - **Exported API is reduced!**: I'm working with szymonrybczak to decouple references from RN CLI packages react-native-community/cli#2021. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D46801501 fbshipit-source-id: 03e58ec37ef34f163a041ea67812191b75811dde

Summary: Pull Request resolved: facebook#38795 ## Context RFC: Decoupling Flipper from React Native core: react-native-community/discussions-and-proposals#641 ## Changes Inits new package `react-native/community-cli-plugin`. This migrates [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) into the React Native repo, to enable faster iteration by the React Native core team. Specifically: - This package contains several `metro` dependencies, which when removed from CLI will no longer require us to ship new CLI releases to get Metro patches and features to users. - This package contains the `start`, `bundle`, and `ram-bundle` commands (central to the React Native development experience), for which we have incoming debugging-related changes. - This package now **only** exports commands to be attached via a RN CLI plugin. With this move, we're aiming to **internalise** the default implementations of these dev commands within React Native — other RN CLI plugins can continue to override these, but must do so wholesale. (See also the recent fix for this: react-native-community/cli#1999.) In V15: - (Microsoft feedback) Re-export `unstable_buildBundleWithConfig`, marking as unstable. This gives us a time buffer to consider how we repackage this functionality in future. The package source has been converted from TypeScript to Flow, with a number of new `flow-typed/` defs added to meet type coverage requirements. ## To dos - For now, we aren't removing the existing [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) source — until later PRs consolidate this move by changing dependencies in the `react-native` package. - **Exported API is reduced!**: I'm working with szymonrybczak to decouple references from RN CLI packages react-native-community/cli#2021. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D46801501 fbshipit-source-id: b25b71677cca0cc1007ddc9f59615e1585b3bf88

Summary: Pull Request resolved: #38795 ## Context RFC: Decoupling Flipper from React Native core: react-native-community/discussions-and-proposals#641 ## Changes Inits new package `react-native/community-cli-plugin`. This migrates [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) into the React Native repo, to enable faster iteration by the React Native core team. Specifically: - This package contains several `metro` dependencies, which when removed from CLI will no longer require us to ship new CLI releases to get Metro patches and features to users. - This package contains the `start`, `bundle`, and `ram-bundle` commands (central to the React Native development experience), for which we have incoming debugging-related changes. - This package now **only** exports commands to be attached via a RN CLI plugin. With this move, we're aiming to **internalise** the default implementations of these dev commands within React Native — other RN CLI plugins can continue to override these, but must do so wholesale. (See also the recent fix for this: react-native-community/cli#1999.) In V15: - (Microsoft feedback) Re-export `unstable_buildBundleWithConfig`, marking as unstable. This gives us a time buffer to consider how we repackage this functionality in future. The package source has been converted from TypeScript to Flow, with a number of new `flow-typed/` defs added to meet type coverage requirements. ## To dos - For now, we aren't removing the existing [`react-native-community/cli-plugin-metro`](https://github.com/react-native-community/cli/tree/main/packages/cli-plugin-metro) source — until later PRs consolidate this move by changing dependencies in the `react-native` package. - **Exported API is reduced!**: I'm working with szymonrybczak to decouple references from RN CLI packages react-native-community/cli#2021. Changelog: [Internal] Reviewed By: motiz88 Differential Revision: D46801501 fbshipit-source-id: 7f6b72941a69f487fb437768cdba125a9aa3418d

huntie requested review from adamTrz and thymikee as code owners July 5, 2023 10:22

github-actions bot added the bugfix label Jul 5, 2023

tido64 reviewed Jul 5, 2023

View reviewed changes

packages/cli/src/index.ts Outdated Show resolved Hide resolved

huntie force-pushed the fix-plugin-system branch from 889f00f to 6047aee Compare July 5, 2023 10:43

huntie mentioned this pull request Jul 6, 2023

Commander unsafely allows commands with duplicate names to be registered tj/commander.js#1903

Closed

shadowspawn reviewed Jul 6, 2023

View reviewed changes

packages/cli/src/index.ts Outdated Show resolved Hide resolved

breaking: Fix ability to override commands via config

f7d1176

huntie force-pushed the fix-plugin-system branch from 6047aee to f7d1176 Compare July 7, 2023 15:22

tido64 approved these changes Jul 17, 2023

View reviewed changes

szymonrybczak approved these changes Jul 18, 2023

View reviewed changes

thymikee merged commit ba362e6 into react-native-community:main Jul 18, 2023

huntie deleted the fix-plugin-system branch July 18, 2023 16:49

huntie mentioned this pull request Aug 7, 2023

Migrate cli-plugin-metro into repo facebook/react-native#38795

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

breaking: Fix ability to override commands via config #1999

breaking: Fix ability to override commands via config #1999

huntie commented Jul 5, 2023

huntie commented Jul 17, 2023

tido64 left a comment

szymonrybczak left a comment

breaking: Fix ability to override commands via config #1999

breaking: Fix ability to override commands via config #1999

Conversation

huntie commented Jul 5, 2023

Summary

Test Plan

huntie commented Jul 17, 2023

tido64 left a comment

Choose a reason for hiding this comment

szymonrybczak left a comment

Choose a reason for hiding this comment