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

Merge rnpm cli into react-native #7899

Closed
wants to merge 73 commits into from
Closed

Merge rnpm cli into react-native #7899

wants to merge 73 commits into from

Conversation

grabbou
Copy link
Contributor

@grabbou grabbou commented Jun 2, 2016

This is an initial step of rewriting the CLI interface to use rnpm one (commander, plugins etc.).

It's scope is to move all existing commands to use rnpm CLI interface, so that we get plugins, flags and our existing ecosystem working out of the box.

This is still WIP and some of the commands are left commented out.

For the config of rnpm (functions get info about project and dependency), I am thinking we can merge them with we decided to merge it with default.config.js, so they are available on the new Config() instance (which means we don't have to change anything and current plugins, like runIOS and runAndroid can just start using it w/o depending on any extra argument).

Roadmap:

  • Unit test all the things ( @Kureev is working on that )
  • Bring back plugins
  • Port missing commands

Test plan:

  1. Clone the project
  2. npm install
  3. node ./local-cli/cli.js --help
  4. View the diff with ?w=0 to ignore indents

Breaking changes for current rnpm developers:

  • the interface for a command did change to align with what is currently in React Native (partially to reduce the diff). See the flow definition for a final list of options allowed,
  • the default option for a flag can now be a function that receives an instance of a Config. I found it a common pattern, e.g. here. If we decide to merge rnpm config into Config, then e.g. here we can just swap default value from ios to (config) => config.getProjectConfig().ios.sourceDir
  • the order of arguments did change a bit (reasoning the same + some of the commands are plain no-config ones), the first one is now argv, the 2nd one is config, which means they are swapped.

There's plenty of stuff yet to be done, but we are trying to keep the diff small. We still have to post new docs and developers guide. @bestander @vjeux since the local-cli is now extensible and supports custom configs (either through rn-cli or via package.json), can we think of an extra section on the website to describe it?

@ghost
Copy link

ghost commented Jun 2, 2016

By analyzing the blame information on this pull request, we identified @janicduplessis and @mroswald to be potential reviewers.

@ghost ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 2, 2016
@ghost
Copy link

ghost commented Jun 2, 2016

@grabbou updated the pull request.

4 similar comments
@ghost
Copy link

ghost commented Jun 2, 2016

@grabbou updated the pull request.

@ghost
Copy link

ghost commented Jun 2, 2016

@grabbou updated the pull request.

@ghost
Copy link

ghost commented Jun 2, 2016

@grabbou updated the pull request.

@facebook-github-bot
Copy link
Contributor

@grabbou updated the pull request.

'init': [printInitWarning, ''],
const handleError = (err) => {
console.error();
console.error(err.message || err);
Copy link
Contributor Author

@grabbou grabbou Jun 2, 2016

Choose a reason for hiding this comment

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

Ideally I'd like to reject promises with an error, not a plain string. But since this is a common catch-all handler, it should support both.

@ghost
Copy link

ghost commented Jun 2, 2016

@grabbou updated the pull request.

const isPackagerRunning = require('../util/isPackagerRunning');
const Promise = require('promise');
const adb = require('./adb');

// Verifies this is an Android project
function checkAndroid(root) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc: @Kureev I can see lots of things for rnpm to do here :)

@grabbou grabbou changed the title Feat/rnpm cli interface Merge rnpm cli into react-native Jun 2, 2016

if (!checkAndroid(args)) {
function runAndroid(argv, config, args) {
if (!checkAndroid(args.root)) {
console.log(chalk.red('Android project not found. Maybe run react-native android first?'));
Copy link
Contributor Author

@grabbou grabbou Jun 2, 2016

Choose a reason for hiding this comment

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

@mkonicek in next PR, what I would like to see is to move these all chalk.red calls (errors) to be just Promise.reject / throw new Error so that we have a centralised error reported in the cliEntry.js file. That would also stay consistent with other commands (like runIOS) where we throw errors instead of logging & swallowing them. Might be also a bit easier to work with in case one is debugging and doesn't know where the error is coming from. What you think? :) Was there any specific motivation for doing it that way?

@ghost
Copy link

ghost commented Jun 3, 2016

@grabbou updated the pull request.

2 similar comments
@ghost
Copy link

ghost commented Jun 3, 2016

@grabbou updated the pull request.

@ghost
Copy link

ghost commented Jun 3, 2016

@grabbou updated the pull request.

@bestander
Copy link
Contributor

Something wrong about generated bundle for instrumentation tests

@grabbou
Copy link
Contributor Author

grabbou commented Jun 3, 2016

Yeah @bestander, bundle wasn't being generated at all (I've disabled that command). Last commit brings it back, maybe it will go green now :)

@ghost
Copy link

ghost commented Jun 3, 2016

@grabbou updated the pull request.

Wrap everything in a promise so that we not only print out on
rejections, but also on other errors that were thrown (e.g. in a
assertRequiredOptions or in a new-library command).

Note: command itself doesn't need to return a promise.
@ghost ghost 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 Jul 28, 2016
@bestander
Copy link
Contributor

@facebook-github-bot import

@bestander
Copy link
Contributor

I'll give it a try tomorrow morning, thanks, Michal

@ghost
Copy link

ghost commented Jul 28, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review internal test results.

@ghost ghost 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 Jul 28, 2016
@bestander
Copy link
Contributor

@grabbou, I was able to get a shim for bundle/unbundle/dependencies

const bundle = require('../../react-native-github/local-cli/bundle/bundle');
const Config = require('../../react-native-github/local-cli/util/Config');
const dependencies = require('../../react-native-github/local-cli/dependencies/dependencies');
const genRouteBuilders = require('./routes/genRouteBuilders');
const mapTargetsToEntryPaths = require('./target-determinator/mapTargetsToEntryPaths');
const routes = require('./routes/routes');
const server = require('./server/server');
const unbundle = require('../../react-native-github/local-cli/bundle/unbundle');
const commander = require('commander');
const Promise = require('promise');

function shimCommander(argv, config, packagerInstance, command) {
  return new Promise((resolve, reject) => {
    commander
      .command(command.name)
      .usage(command.examples)
      .description(command.description)
      .action(function runAction() {
        command.func(argv, config, this.opts(), packagerInstance)
          .then(resolve, reject);
      });
    command.options
      .forEach(opt => commander.option(
        opt.command,
        opt.description,
        opt.parse,
        typeof opt.default === 'function' ? opt.default(config) : opt.default,
      ));
    commander.parse(argv);
  });
}

const internalCommands = {
  genRouteBuilders: genRouteBuilders,
  mapTargetsToEntryPaths: mapTargetsToEntryPaths,
  routes: routes,
  server: server,
  bundle: (argv, config, packagerInstance) => {
    return shimCommander(argv, config, packagerInstance, bundle);
  },
  unbundle: (argv, config, packagerInstance) => {
    return shimCommander(argv, config, packagerInstance, unbundle);
  },
  dependencies: (argv, config, packagerInstance) => {
    return shimCommander(argv, config, packagerInstance, dependencies);
  },
};

function run(pwd, command, commandArgs, packagerInstance) {
  if (!command) {
    throw new Error(helpMessage());
  }
  commandArgs = commandArgs || [];

  var commandToExec = internalCommands[command];
  if (!commandToExec) {
    throw new Error(helpMessage(command));
  }

  return commandToExec(commandArgs, Config.get(pwd), packagerInstance);
}

But I have a feeling that .action is never executed.
Is there anything I need to call to kick off commander to execute it?

Also we plan to opensource this internal CLI code soon.

@ghost ghost 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 Jul 29, 2016
@bestander
Copy link
Contributor

Ok, today I was able to get a bit further.
Commander.js is quite confusing, being stateful it is hard to use it in the middle of the application to shim things.
I'll once more on Monday

@grabbou
Copy link
Contributor Author

grabbou commented Jul 29, 2016

@bestander I believe it would be much simpler to just call the functions itself directly, though I don't want to introduce extra mess here w/o having an access to the code you are trying to shim. Hoping we can get it merged sooner than the CLI wrappers being open sourced, otherwise we'll have to wait.

@ghost ghost 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 Jul 29, 2016
@bestander
Copy link
Contributor

Good news, I was able to get the wrappers working.
Just a few minor infra issues and the PR will be merged.

A few rnpm Jest tests are failing

Summary of all failing tests
 FAIL  react-native-github/local-cli/rnpm/link/__tests__/link.spec.js (1.161s)
● link › it should read dependencies from package.json when name not provided
  - Error: Cannot find module '/data/sandcastle/boxes/instance-fbobjc-fbsource-catalyst-jest-macosx-local/fbobjc/Libraries/FBReactKit/package.json' from 'link.spec.js'
        at Resolver.resolveModule (react-native-github/node_modules/jest-resolve/build/index.js:151:17)
        at Object.<anonymous> (react-native-github/local-cli/rnpm/link/__tests__/link.spec.js:47:10)

 FAIL  react-native-github/local-cli/rnpm/core/__tests__/findPlugins.spec.js (0.159s)
● findPlugins › it should return an array of dependencies
  - Error: Cannot find module '/data/sandcastle/boxes/instance-fbobjc-fbsource-catalyst-jest-macosx-local/fbobjc/Libraries/FBReactKit/package.json' from 'findPlugins.spec.js'
        at Resolver.resolveModule (react-native-github/node_modules/jest-resolve/build/index.js:151:17)
        at Object.<anonymous> (react-native-github/local-cli/rnpm/core/__tests__/findPlugins.spec.js:13:10)
● findPlugins › it should return an empty array if there're no plugins in this folder
  - Error: Cannot find module '/data/sandcastle/boxes/instance-fbobjc-fbsource-catalyst-jest-macosx-local/fbobjc/Libraries/FBReactKit/package.json' from 'findPlugins.spec.js'
        at Resolver.resolveModule (react-native-github/node_modules/jest-resolve/build/index.js:151:17)
        at Object.<anonymous> (react-native-github/local-cli/rnpm/core/__tests__/findPlugins.spec.js:21:10)
● findPlugins › it should return plugins from both dependencies and dev dependencies
  - Error: Cannot find module '/data/sandcastle/boxes/instance-fbobjc-fbsource-catalyst-jest-macosx-local/fbobjc/Libraries/FBReactKit/package.json' from 'findPlugins.spec.js'
        at Resolver.resolveModule (react-native-github/node_modules/jest-resolve/build/index.js:151:17)
        at Object.<anonymous> (react-native-github/local-cli/rnpm/core/__tests__/findPlugins.spec.js:31:10)
● findPlugins › it should return unique list of plugins
  - Error: Cannot find module '/data/sandcastle/boxes/instance-fbobjc-fbsource-catalyst-jest-macosx-local/fbobjc/Libraries/FBReactKit/package.json' from 'findPlugins.spec.js'
        at Resolver.resolveModule (react-native-github/node_modules/jest-resolve/build/index.js:151:17)
        at Object.<anonymous> (react-native-github/local-cli/rnpm/core/__tests__/findPlugins.spec.js:39:10)

 FAIL  react-native-github/local-cli/rnpm/link/__tests__/getProjectDependencies.spec.js (0.02s)
● getProjectDependencies › it should return an array of project dependencies
  - Error: Cannot find module '/data/sandcastle/boxes/instance-fbobjc-fbsource-catalyst-jest-macosx-local/fbobjc/Libraries/FBReactKit/package.json' from 'getProjectDependencies.spec.js'
        at Resolver.resolveModule (react-native-github/node_modules/jest-resolve/build/index.js:151:17)
        at Object.<anonymous> (react-native-github/local-cli/rnpm/link/__tests__/getProjectDependencies.spec.js:11:10)
● getProjectDependencies › it should return an empty array when no dependencies set
  - Error: Cannot find module '/data/sandcastle/boxes/instance-fbobjc-fbsource-catalyst-jest-macosx-local/fbobjc/Libraries/FBReactKit/package.json' from 'getProjectDependencies.spec.js'
        at Resolver.resolveModule (react-native-github/node_modules/jest-resolve/build/index.js:151:17)
        at Object.<anonymous> (react-native-github/local-cli/rnpm/link/__tests__/getProjectDependencies.spec.js:20:10)

Any ideas?
Those tests are running from another folder.

@Kureev
Copy link
Contributor

Kureev commented Jul 30, 2016

Seems you don't have a proper package.json in the folder, where you run CLI

@bestander
Copy link
Contributor

Thanks, I'll copy the changes

On Saturday, 30 July 2016, Alexey notifications@github.com wrote:

Seems you don't have a proper package.json in the folder, where you run CLI


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7899 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACBdWJ_ZhfrXuxRoNUNcHNJSIuwQzeQVks5qax3PgaJpZM4Is8X3
.

@ghost
Copy link

ghost commented Jul 30, 2016

@grabbou updated the pull request.

1 similar comment
@ghost
Copy link

ghost commented Jul 30, 2016

@grabbou updated the pull request.

@ghost ghost 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 Jul 30, 2016
@bestander
Copy link
Contributor

Ok, I decided to disable rnpm tests from running at FB CI for now.
For a while this will be the community responsibility and we will try not to break it from inside.

@grabbou, @Kureev, could you think of a way to decouple rnpm from CWD in the next few releases?

Brace yourselves, this PR is going to be landed now

@ghost ghost 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 Jul 30, 2016
grabbou added a commit that referenced this pull request Aug 1, 2016
Summary:
This is an initial step of rewriting the CLI interface to use `rnpm` one (commander, plugins etc.).

It's scope is to move all existing commands to use rnpm CLI interface, so that we get plugins, flags and our existing ecosystem working out of the box.

<s>This is still WIP and some of the commands are left commented out.</s>

For the `config` of `rnpm` (functions get info about project and dependency), <s>I am thinking we can merge them with</s> we decided to merge it with [`default.config.js`](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/default.config.js#L33), so they are available on the `new Config()` [instance](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/cliEntry.js#L59) (which means we don't have to change anything and current plugins, like runIOS and runAndroid can just start using it [w/o depending on any extra argument](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e
Closes #7899

Differential Revision: D3613193

Pulled By: bestander

fbshipit-source-id: 09a072f3b21e5239dfcd8da88a205bd28dc5d037
bartolkaruza pushed a commit to immidi/react-native that referenced this pull request Aug 3, 2016
Summary:
This is an initial step of rewriting the CLI interface to use `rnpm` one (commander, plugins etc.).

It's scope is to move all existing commands to use rnpm CLI interface, so that we get plugins, flags and our existing ecosystem working out of the box.

<s>This is still WIP and some of the commands are left commented out.</s>

For the `config` of `rnpm` (functions get info about project and dependency), <s>I am thinking we can merge them with</s> we decided to merge it with [`default.config.js`](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/default.config.js#L33), so they are available on the `new Config()` [instance](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/cliEntry.js#L59) (which means we don't have to change anything and current plugins, like runIOS and runAndroid can just start using it [w/o depending on any extra argument](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e
Closes facebook#7899

Differential Revision: D3613193

Pulled By: bestander

fbshipit-source-id: 09a072f3b21e5239dfcd8da88a205bd28dc5d037
samerce pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
This is an initial step of rewriting the CLI interface to use `rnpm` one (commander, plugins etc.).

It's scope is to move all existing commands to use rnpm CLI interface, so that we get plugins, flags and our existing ecosystem working out of the box.

<s>This is still WIP and some of the commands are left commented out.</s>

For the `config` of `rnpm` (functions get info about project and dependency), <s>I am thinking we can merge them with</s> we decided to merge it with [`default.config.js`](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/default.config.js#L33), so they are available on the `new Config()` [instance](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/cliEntry.js#L59) (which means we don't have to change anything and current plugins, like runIOS and runAndroid can just start using it [w/o depending on any extra argument](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e
Closes facebook#7899

Differential Revision: D3613193

Pulled By: bestander

fbshipit-source-id: 09a072f3b21e5239dfcd8da88a205bd28dc5d037
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
This is an initial step of rewriting the CLI interface to use `rnpm` one (commander, plugins etc.).

It's scope is to move all existing commands to use rnpm CLI interface, so that we get plugins, flags and our existing ecosystem working out of the box.

<s>This is still WIP and some of the commands are left commented out.</s>

For the `config` of `rnpm` (functions get info about project and dependency), <s>I am thinking we can merge them with</s> we decided to merge it with [`default.config.js`](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/default.config.js#L33), so they are available on the `new Config()` [instance](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/cliEntry.js#L59) (which means we don't have to change anything and current plugins, like runIOS and runAndroid can just start using it [w/o depending on any extra argument](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e
Closes facebook#7899

Differential Revision: D3613193

Pulled By: bestander

fbshipit-source-id: 09a072f3b21e5239dfcd8da88a205bd28dc5d037
philikon added a commit to philikon/react-native that referenced this pull request Sep 7, 2016
Restores feature introduced in facebook#7961 after it's been paved partially by facebook#7899
philikon added a commit to philikon/react-native that referenced this pull request Sep 8, 2016
Restores feature introduced in facebook#7961 after it's been paved partially by facebook#7899
ghost pushed a commit that referenced this pull request Sep 12, 2016
Summary:
Restores feature introduced in #7961 after it's been paved partially by #7899

**Test Plan:** ran example in https://github.com/philikon/ReactNativify against a React Native with and without this patch
Closes #9799

Differential Revision: D3852601

Pulled By: mkonicek

fbshipit-source-id: fc3c80bdb254145fefa870eea1828b4ef33f9297
miklschmidt added a commit to secoya/react-native that referenced this pull request Oct 7, 2016
Summary:
Restores feature introduced in facebook#7961 after it's been paved partially by facebook#7899

**Test Plan:** ran example in https://github.com/philikon/ReactNativify against a React Native with and without this patch
Closes facebook#9799

Differential Revision: D3852601

Pulled By: mkonicek

fbshipit-source-id: fc3c80bdb254145fefa870eea1828b4ef33f9297
grabbou added a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
This is an initial step of rewriting the CLI interface to use `rnpm` one (commander, plugins etc.).

It's scope is to move all existing commands to use rnpm CLI interface, so that we get plugins, flags and our existing ecosystem working out of the box.

<s>This is still WIP and some of the commands are left commented out.</s>

For the `config` of `rnpm` (functions get info about project and dependency), <s>I am thinking we can merge them with</s> we decided to merge it with [`default.config.js`](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/default.config.js#L33), so they are available on the `new Config()` [instance](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e70adb69675786/local-cli/cliEntry.js#L59) (which means we don't have to change anything and current plugins, like runIOS and runAndroid can just start using it [w/o depending on any extra argument](https://github.com/grabbou/react-native/blob/e57683e420210749a5a6b802b4e
Closes facebook/react-native#7899

Differential Revision: D3613193

Pulled By: bestander

fbshipit-source-id: 09a072f3b21e5239dfcd8da88a205bd28dc5d037
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants