Extract CLI out #22174

grabbou · 2018-11-06T13:11:24Z

Removes CLI and uses react-native-local-cli instead. Opening this PR to make sure tests are green and that all requires to local-cli are either removed or replaced with a new version.

Right now, the react-native-local-cli is synced with local-cli folder present in 0.57.0 of React Native. I will work on upgrading that shortly as we tried to make minimal changes to the CLI before it actually gets extracted.

Next steps:

Make sure react-native init works and new projects work w/o issues
CI is green
All call-sites to local-cli are replaced
Sync with latest master
Ship it!

grabbou · 2018-11-06T13:18:05Z

Note: Android tests seem to be failing anyway - I am ignoring them locally on my machine too. Will report back once I find out. It's likely because this is super fresh PR based on latest master.

grabbou · 2018-11-06T14:49:15Z

react-native init works without issues. Latest updates required to make it work are present in CLI repository master branch.

grabbou · 2018-11-06T16:07:40Z

CLI is now updated to the latest commit. I am going to work on it tomorrow. I need to set up proper CI tests for the CLI itself and make sure all the Flow/Unit tests are passing w/o issues.

We are moving in the right direction.

elicwhite · 2018-11-06T21:13:14Z

package.json

@@ -191,6 +191,7 @@
    "prop-types": "^15.5.8",
    "react-clone-referenced-element": "^1.0.1",
    "react-devtools-core": "^3.4.0",
+    "react-native-local-cli": "^0.57.0",


Do you expect that the CLI and react-native will continue to be versioned together?

To be defined. Did it really quick to demonstrate it on the CI. Since I don't plan on releasing CLI exactly at the time of the new React Native release (in fact, I would like to do it more often to fix critical issues faster and ship new improvements), it will be really hard to keep up the same number and not to cause confusion.

I guess we can start with 1.0.0 when the migration is over and the PR is ready.

I agree with having separate versioning, but this means that the new repo must have a table of compatibility of some sort

Don't think it's needed at the beginning since React Native contains a dependency on CLI for now. It is similar to the way we handle Metro - it also doesn't have compatibility table on React Native.

However, I do think keeping a peerDependency is going to be the right choice to make it easier to sync CLI and React Native versions together.

FYI I already published 1.0.0-alpha.0 version and updated this PR.

…require its files

grabbou · 2018-11-07T14:31:58Z

Libraries/Image/assetPathUtils.js

+ * FIXME: using number to represent discrete scale numbers is fragile in essence because of
+ * floating point numbers imprecision.
+ */
+function getAndroidAssetSuffix(scale: number): string {


This file is now duplicated between React Native and React Native CLI. Shall we consider making it a separate package, e.g. react-native-cli-path-utils and publish to npm?

CC: @TheSavior

I feel that to keep RN and the CLI separated but having them both using these utils the separate package option is a valid solution

Yeah. For now, I think we can ship the copy and figure out this as a next PR. Problem with this one is that I need to update local-cli folder every time something changes which is very hard. Hopefully, we can get this reviewed ASAP.

Note that this file is for more than just images, so it does need to be in RN, to be used by other asset types.

grabbou · 2018-11-08T15:03:55Z

Update: There's one last issue I need to fix related to Metro Bundler. Once that happens, this will be ready for review. I am hoping to do it by today.

grabbou · 2018-11-09T11:05:07Z

This pull request is ready to be reviewed.

I have tested it by:
a) running most of the commands manually confirming they work
b) making sure all tests pass (Flow & Jest)
c) running ./scripts/test-manual-e2e.sh and initing an RNDemoApp project to use the new CLI tools. It run w/o issues by invoking yarn react-native run-ios and yarn react-native start.

Migration strategy:

If you have a require call to local-cli folder, replace it with react-native-local-cli or consider removing entirely.

For every new release, we are going to verify that CLI works (by running step "c" from the above, which is exactly the same workflow we used to have) and if there are no issues, the peerDependency will be updated in the CLI or new version released to indicate the support. I expect small changes to happen as we put this into practice.

To Metro team:
Stick to your current approach where you bump Metro dependencies in React Native master and we will be following you inside react-native-cli. Ideally, you'd send a PR to this repository with bumped versions too and required changes to the API if any.

This is non-breaking and transparent change to users unless they were requiring files from local-cli (private API, not guaranteed to work). In this case, they just need to update imports.

The sooner we merge it, the easier it will be for us to keep up with changes that happen to CLI itself and React Native.

grabbou · 2018-11-09T12:07:34Z

Note: We don't have to ship it in next RC. This commit can be easily excluded if it causes any concerns. It can also be easily cherry-picked to the next version.

grabbou · 2018-11-09T13:51:49Z

Note: Once the PR is merged, I will work on:

all open PRs and ask authors to submit the same diff against react-native-cli instead
all open issues and move them to the react-native-cli where it makes sense

grabbou · 2018-11-09T14:56:39Z

Tests are green. Please ignore Appveyor error - I don't know why it happens.

axe-fb · 2018-11-16T22:16:12Z

local-cli/cli.js

-require('../setupBabel')();
-
-var cliEntry = require('./cliEntry');
+var cli = require('react-native-local-cli');


(nit): why change the name of the variable ?

I ended up overwriting this file with a local copy I created upfront. Not intentional, I can revert if needed.

axe-fb · 2018-11-16T22:39:16Z

Libraries/Image/AssetSourceResolver.js

@@ -22,7 +22,7 @@ import type {PackagerAsset} from 'AssetRegistry';
 const PixelRatio = require('PixelRatio');
 const Platform = require('Platform');

-const assetPathUtils = require('../../local-cli/bundle/assetPathUtils');


assetPathUtils are not limited to images. They also impact all other types of assets.

Right, but my understanding is that the "impact" is more present in the local-cli context where the utils are used to generate a path to any type of asset.

On the React Native side of things, this file is only used in Image module. Anyway, this shouldn't be an issue now that the local-cli folder will not be removed. I am going to revert this change and w can leave this to be refactored at some point.

axe-fb · 2018-11-16T22:41:34Z

Libraries/Image/assetPathUtils.js

@@ -10,7 +10,7 @@

 'use strict';

-import type {PackagerAsset} from '../../Libraries/Image/AssetRegistry';


Not sure if moving this utility and making it specific to Image makes sense. This is for all assets. Maybe this should be a separate util.

Again, not an issue that we leave local-cli. However, this is only used in Image module on the React Native side.

grabbou · 2018-11-18T17:50:48Z

Closing in favor of #22337. We have decided to not remove local-cli folder, for now, to not break the code that relies on its private API and might be using it in some places.

Summary: Continuation of #22174 with an exception that `local-cli` folder is left in React Native repository to keep Facebook internal and React Native calls still working. Separate strategy should be developed to remove all uses of `local-cli` in favor of dedicated utilities. Pull Request resolved: #22337 Reviewed By: TheSavior Differential Revision: D13172898 Pulled By: cpojer fbshipit-source-id: 0217867f9944648307475ebe629eb729da7bfaaf

Summary: Continuation of facebook/react-native#22174 with an exception that `local-cli` folder is left in React Native repository to keep Facebook internal and React Native calls still working. Separate strategy should be developed to remove all uses of `local-cli` in favor of dedicated utilities. Pull Request resolved: facebook/react-native#22337 Reviewed By: TheSavior Differential Revision: D13172898 Pulled By: cpojer fbshipit-source-id: 0217867f9944648307475ebe629eb729da7bfaaf

JAStanton · 2018-12-13T05:02:02Z

@grabbou excuse my ignorance, but I see that local-cli has been removed in favor of react-native-local-cli which is published on npm but I cannot find the source anywhere? Is it in source control somewhere?

kelset · 2018-12-13T09:31:26Z

@JAStanton you can find the repo here -> https://github.com/react-native-community/react-native-cli

And the original proposal -> react-native-community/discussions-and-proposals#13

JAStanton · 2018-12-13T15:14:06Z

Thank you!

…

On Thu, Dec 13, 2018 at 1:31 AM Lorenzo Sciandra ***@***.***> wrote: @JAStanton <https://github.com/JAStanton> you can find the repo here -> https://github.com/react-native-community/react-native-cli And the original proposal -> react-native-community/discussions-and-proposals#13 <react-native-community/discussions-and-proposals#13> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22174 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnYtJZ1uZedmzKVqnn0_9ABi8CmZGxDks5u4h5-gaJpZM4YQakz> .

Summary: This sync includes the following changes: - **[95d762e40](facebook/react@95d762e40 )**: Remove duplicate test //<Andrew Clark>// - **[d4d1dc085](facebook/react@d4d1dc085 )**: Reorder VARIANT feature flags ([#22266](facebook/react#22266)) //<Dan Abramov>// - **[2f156eafb](facebook/react@2f156eafb )**: Adjust consoleManagedByDevToolsDuringStrictMode feature flag ([#22253](facebook/react#22253)) //<Dan Abramov>// - **[cfd819332](facebook/react@cfd819332 )**: Add useSyncExternalStore to react-debug-tools ([#22240](facebook/react#22240)) //<Andrew Clark>// - **[8e80592a3](facebook/react@8e80592a3 )**: Remove state queue from useSyncExternalStore ([#22265](facebook/react#22265)) //<Andrew Clark>// - **[06f98c168](facebook/react@06f98c168 )**: Implement useSyncExternalStore in Fiber ([#22239](facebook/react#22239)) //<Andrew Clark>// - **[77912d9a0](facebook/react@77912d9a0 )**: Wire up the native API for useSyncExternalStore ([#22237](facebook/react#22237)) //<Andrew Clark>// - **[031abd24b](facebook/react@031abd24b )**: Add warning and test for useSyncExternalStore when getSnapshot isn't cached ([#22262](facebook/react#22262)) //<salazarm>// - **[b8884de24](facebook/react@b8884de24 )**: break up import keyword to avoid being accidentally parsed as dynamic import statement in external code ([#21918](facebook/react#21918)) //<Jianhua Zheng>// - **[6d6bba5bf](facebook/react@6d6bba5bf )**: Fix typo in ReactUpdatePriority-test.js ([#21958](facebook/react#21958)) //<Ikko Ashimine>// - **[0c0d1ddae](facebook/react@0c0d1ddae )**: feat(eslint-plugin-react-hooks): support ESLint 8.x ([#22248](facebook/react#22248)) //<Michaël De Boey>// - **[1314299c7](facebook/react@1314299c7 )**: Initial shim of useSyncExternalStore ([#22211](facebook/react#22211)) //<Andrew Clark>// - **[fc40f02ad](facebook/react@fc40f02ad )**: Add consoleManagedByDevToolsDuringStrictMode feature flag in React Reconciler ([#22196](facebook/react#22196)) //<Luna Ruan>// - **[46a0f050a](facebook/react@46a0f050a )**: Set up use-sync-external-store package ([#22202](facebook/react#22202)) //<Andrew Clark>// - **[8723e772b](facebook/react@8723e772b )**: Fix a string interpolation typo in ReactHooks test ([#22174](facebook/react#22174)) //<Matt Hargett>// - **[60a30cf32](facebook/react@60a30cf32 )**: Console Logging for StrictMode Double Rendering ([#22030](facebook/react#22030)) //<Luna Ruan>// - **[76bbad3e3](facebook/react@76bbad3e3 )**: Add maxYieldMs feature flag in Scheduler ([#22165](facebook/react#22165)) //<Ricky>// - **[b0b53ae2c](facebook/react@b0b53ae2c )**: Add feature flags for scheduler experiments ([#22105](facebook/react#22105)) //<Ricky>// Changelog: [General][Changed] - React Native sync for revisions bd5bf55...95d762e jest_e2e[run_all_tests] Reviewed By: mdvacca Differential Revision: D30809906 fbshipit-source-id: 131cfdf91e15f67fa59a5d925467e538ee89fe10

Extract CLI out and fix call-sites (some of them)

b84afe6

grabbou requested review from hramos, Kureev and shergin as code owners November 6, 2018 13:11

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. ✅pr adds tests labels Nov 6, 2018

Return CLI for global CLI

bf36177

react-native-bot added the Core Team label Nov 6, 2018

elicwhite reviewed Nov 6, 2018

View reviewed changes

grabbou added 3 commits November 7, 2018 14:56

Fix tests - transform new location of CLI

ecb6d98

Move bin property to react-native-local-cli

47e0658

Fix Jest tests - move assetPathUtils from CLI to here, RN should not …

586f54f

…require its files

grabbou commented Nov 7, 2018

View reviewed changes

Bump dependency on React Native for review

5373544

Bump CLI version for review

af5e7cf

grabbou added 2 commits November 9, 2018 14:19

Bump CLI dependency to fix tests

c200278

Fix CLI version

83ae4ba

Bump CLI dependency

2287be4

hramos removed the ✅pr adds tests label Nov 14, 2018

axe-fb reviewed Nov 16, 2018

View reviewed changes

grabbou mentioned this pull request Nov 18, 2018

Extract out CLI #22337

Closed

grabbou closed this Nov 18, 2018

zpao deleted the feat/extract-cli branch January 31, 2019 01:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract CLI out #22174

Extract CLI out #22174

grabbou commented Nov 6, 2018 •

edited

Loading

grabbou commented Nov 6, 2018

grabbou commented Nov 6, 2018

grabbou commented Nov 6, 2018

elicwhite Nov 6, 2018

grabbou Nov 7, 2018

kelset Nov 8, 2018

grabbou Nov 8, 2018

grabbou Nov 8, 2018

grabbou Nov 7, 2018

kelset Nov 8, 2018

grabbou Nov 8, 2018

axe-fb Nov 16, 2018

grabbou commented Nov 8, 2018

grabbou commented Nov 9, 2018

grabbou commented Nov 9, 2018

grabbou commented Nov 9, 2018

grabbou commented Nov 9, 2018

axe-fb Nov 16, 2018

grabbou Nov 18, 2018

axe-fb Nov 16, 2018

grabbou Nov 18, 2018

axe-fb Nov 16, 2018

grabbou Nov 18, 2018

grabbou commented Nov 18, 2018

JAStanton commented Dec 13, 2018

kelset commented Dec 13, 2018

JAStanton commented Dec 13, 2018 via email

		@@ -10,7 +10,7 @@

		'use strict';

		import type {PackagerAsset} from '../../Libraries/Image/AssetRegistry';

Extract CLI out #22174

Extract CLI out #22174

Conversation

grabbou commented Nov 6, 2018 • edited Loading

grabbou commented Nov 6, 2018

grabbou commented Nov 6, 2018

grabbou commented Nov 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grabbou commented Nov 8, 2018

grabbou commented Nov 9, 2018

grabbou commented Nov 9, 2018

grabbou commented Nov 9, 2018

grabbou commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grabbou commented Nov 18, 2018

JAStanton commented Dec 13, 2018

kelset commented Dec 13, 2018

JAStanton commented Dec 13, 2018 via email

grabbou commented Nov 6, 2018 •

edited

Loading