Use Yarn for Install/Uninstall CLI if available #11174

n3tr · 2016-11-28T17:57:00Z

Make react-native install and uninstall cli to use yarn if available (fixes #11122)

Test plan Yarn

Publish to Sinopia: https://github.com/facebook/react-native/tree/master/react-native-cli
react-native init AwesomeApp
react-native install react-native-fit-image
react-native uninstall react-native-fit-image

Test plan NPM

Publish to Sinopia: https://github.com/facebook/react-native/tree/master/react-native-cli
react-native init AwesomeAppNPM --npm
react-native install react-native-fit-image
react-native uninstall react-native-fit-image

output (yarn.lock)

> react-native install react-native-fit-image 
yarn add v0.16.1
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
success Saved lockfile.
success Saved 1 new dependency
└─ react-native-fit-image@1.4.6
✨  Done in 4.11s.
rnpm-install info Module react-native-fit-image has been successfully installed & linked

> react-native uninstall react-native-fit-image
yarn remove v0.16.1
[1/2] Removing module react-native-fit-image...
[2/2] Regenerating lockfile and installing missing dependencies...
success Uninstalled packages.
✨  Done in 3.32s.
rnpm-install info Module react-native-fit-image has been successfully uninstalled & unlinked

output (without yarn.lock)

› react-native install react-native-fit-image 
TestApp@0.0.1 /Users/n3tr/Code/TestApp
└── react-native-fit-image@1.4.6 

rnpm-install info Module react-native-fit-image has been successfully installed & linked

› react-native uninstall react-native-fit-image
- react-native-fit-image@1.4.6 node_modules/react-native-fit-image
rnpm-install info Module react-native-fit-image has been successfully uninstalled & unlinked

facebook-github-bot · 2016-11-28T17:57:07Z

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

mkonicek · 2016-11-29T16:01:43Z

local-cli/util/yarn.js

@@ -8,6 +8,7 @@
 */
 'use strict';

+const execSync = require('child_process').execSync;


nit: Probably not required since it's not used in this file.

it's used in line 25 and 27.

https://github.com/facebook/react-native/pull/11174/files/48039bd0dd7404d53e27af458bd73691f81048a9#diff-549f6559732f42d8d33684b24c5cea46R25

For more detail,

As I understand, init/init uses yarn.getYarnVersionIfAvailable() to get version and it will return null from catch block due to execSync is not defined

This will walk you through creating a new React Native project in /Users/n3tr/Code/RN/AwesomeApp Using yarn v0.16.1 Installing react-native... # via yarn Setting up new React Native app in /Users/n3tr/Code/RN/AwesomeApp Installing React... # via npm: from npm install output (init/init.js:80) Installing Jest...

w00t, good catch!

mkonicek · 2016-11-29T16:02:20Z

local-cli/install/install.js

+    yarn.getYarnVersionIfAvailable() &&
+    yarn.isGlobalCliUsingYarn(projectDir);
+
+  var res;


nit: You could use let here.

mkonicek · 2016-11-29T16:06:58Z

local-cli/install/install.js

@@ -10,7 +11,17 @@ log.heading = 'rnpm-install';
 function install(args, config) {
  const name = args[0];

-  var res = spawnSync('npm', ['install', name, '--save'], spawnOpts);
+  const projectDir = config.getProjectRoots()[0];


Is config.getProjectRoots() guaranteed to return an array with at least one element? What if it doesn't, or what if it starts returning undefined under some circumstances in the future? Better check for it just to be safe.

Is this the standard way we use to get the current project dir everywhere else in the CLI, or are there other ways that might be more reliable? Is it possible that people override getProjectRoots in their config to return something else than the folder you'd actually want to use here in the isGlobalCliUsingYarn check? The projectDir here should always be something like ~/MyApp, the same folder where package.json lives, correct?

Just asking some basic questions, I'm not familiar with how the config works.

You're correct.

config.getProjectRoots() can be overridden by user in rn-cli.config.js which can return anything.

I just read through the cli code. I think we can simply use process.cwd() to get projectDir since react-native-cli/index.js already has prevented us from running react-native <commend> outside project directory (contains ./node_modules/react-native/cli.js), What you think?

If process.cwd() works and it's what the rest of the CLI uses I think it sounds good.

mkonicek · 2016-11-29T16:07:36Z

This is awesome, thank you so much for doing it! Commented with just some small nit and questions.

grabbou · 2016-11-29T20:22:18Z

local-cli/install/install.js

+  if (isYarnAvailable) {
+    res = spawnSync('yarn', ['add', name], spawnOpts);
+  } else {
+    res = spawnSync('npm', ['install', name, '--save'], spawnOpts);


Aren't we like including that piece of functionality in many places? Wouldn't it be easier to just have a spawnSync util, called callYarnOrNpm that abstracts that? There are few other places where its going to be needed and so I'd like to keep it as minimal as possible.

I'm thinking as well but still have some questions:

How to handle the different command of yarn [add] and npm [install --save] ?

Should it accept an option to allows to force use the npm client (like in react-native-cli/index)?

What if I make a function call look like:

callYarnOrNpm('add some-package', 'install some-package --save', options);

and let it decides which command will be executed.

manager.add('some-package') // translates to `yarn add some-package` with yarn & `npm install some-package --save` with npm

and init manager with

const manager = new Manager({engine: isYarnInstalled ? 'yarn' : 'npm'});

then move this ^ to the separated file and export an instance of the manager.

Btw, great job!

Maybe I'm thinking too much, I will try to make it simple and improve later depends on further use case. 🤒

First of all, thank you for a really nice feedback ❤️. I think I can help addressing this one:

4 - RARE CASE: Caller want to stick with npm

like react-native-cli/index which allows user can init project with --npm flag and it's happen only before react-native have been installed - it doesn't use local-cli during its process. btw I don't think we need to support --npm flag in local-cli right now.

I think it won't be complicated to check if the current repo has been scaffolded using --npm flag or not. It can be added to the manager's constructor, right next to the check if user have yarn installed. What do you think?

Is it necessary to create manager class and export its instance? IMO, wouldn't it be simpler to just exports a plain object with functions?

module.exports = { add: add, remove: remove }

and I'm not sure what's the filename will be used for manager? is PackageManager good enough?

PackageManager is absolutely fine! The reason why I want to export an instance is that you don't want to check which package manager user is using every time. But at the same moment I'm not sure if we use any command more than once during the CLI run

You know, I just checked that we don't have any places where we call packageManager 2 or more times, so I'm fine with a plain object 😉

Requesting changes because of config.getProjectRoots()[0].

mkonicek · 2016-12-05T13:57:39Z

Requesting changes because of config.getProjectRoots()[0].

Thanks for the review @Kureev!

@n3tr, @Kureev I just noticed there is this line in install.js:

res = spawnSync('rnpm', ['link', name], spawnOpts);

Should it be replaced by something else, e.g. calling into link.js?

Rnpm is now part of the CLI and I think 'rnpm link' is going to fail for most people as they don't have rnpm installed?
https://github.com/rnpm/rnpm

Sorry about the delay reviewing this!

n3tr · 2016-12-05T18:49:13Z

@mkonicek the latest commits no longer use config.getProjectRoots()[0] 😄

I have tested by remove rnpm and run install - the line

res = spawnSync('rnpm', ['link', name], spawnOpts);

It doesn't throw any errors and also doesn't link any dependencies which isn't what we expected.

I'll look into it sometime this week.

Kureev · 2016-12-08T14:52:45Z

Maybe I'm wrong, but rnpm has been renamed to react-native, so it should work if you change it to

res = spawnSync('rnpm', ['link', name], spawnOpts);

About throwing an error: spawnSync doesn't throw, it returns an object with the error instead.

n3tr · 2016-12-08T17:38:19Z

@Kureev I think you're correct.

I've updated the code to

res = spawnSync('react-native', ['link', name], spawnOpts);

also update link.js and unlink.js to support link/unlink with some-package@version.

› react-native install react-native-image-picker@latest  
yarn add v0.16.1
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
[3/4] 🔗  Linking dependencies...
[4/4] 📃  Building fresh packages...
success Saved lockfile.
success Saved 1 new dependency
└─ react-native-image-picker@0.24.1
✨  Done in 3.54s.
rnpm-install info Linking react-native-image-picker android dependency 
rnpm-install info Android module react-native-image-picker has been successfully linked 
rnpm-install info Linking react-native-image-picker ios dependency 
rnpm-install info iOS module react-native-image-picker has been successfully linked 
rnpm-install info Module react-native-image-picker@latest has been successfully installed & linked

Kureev · 2016-12-09T16:15:59Z

Awesome! You rock! Do you have anything else blocking this PR?

n3tr · 2016-12-09T17:35:47Z

@Kureev No, It looks good to me. 😃

Kureev

LGTM 👍

facebook-github-bot · 2017-01-20T00:48:19Z

Something went wrong when importing this pull request. Please cc someone from the team at fb to help with importing this.

Kureev · 2017-02-02T09:06:48Z

I think this branch should be rebased

- Use yarn if available - Fix lint warning

- PackageManager.add(‘some-package’)

- Support install some-package@version

n3tr · 2017-02-02T09:57:22Z

@Kureev rebased :)

facebook-github-bot · 2017-02-02T10:45:16Z

@bestander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-02-02T11:46:55Z

@mkonicek has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Make `react-native install` and `uninstall` cli to use yarn if available (fixes facebook#11122) **Test plan Yarn** 1. Publish to Sinopia: https://github.com/facebook/react-native/tree/master/react-native-cli 2. react-native init AwesomeApp 3. react-native install react-native-fit-image 4. react-native uninstall react-native-fit-image **Test plan NPM** 1. Publish to Sinopia: https://github.com/facebook/react-native/tree/master/react-native-cli 2. react-native init AwesomeAppNPM --npm 3. react-native install react-native-fit-image 4. react-native uninstall react-native-fit-image **output (yarn.lock)** ``` > react-native install react-native-fit-image yarn add v0.16.1 [1/4] 🔍 Resolving packages... [2/4] 🚚 Fetching packages... [3/4] 🔗 Linking dependencies... [4/4] 📃 Building fresh packages... success Saved lockfile. success Saved 1 new dependency └─ react-native-fit-image@1.4.6 ✨ Done in 4.11s. rnpm-install info Module react-native-fit-image has been successfully Closes facebook#11174 Differential Revision: D4441872 fbshipit-source-id: 05bd30e1ad14bdbe861259c88e1f18df77cfa54f

Summary: Make `react-native install` and `uninstall` cli to use yarn if available (fixes #11122) **Test plan Yarn** 1. Publish to Sinopia: https://github.com/facebook/react-native/tree/master/react-native-cli 2. react-native init AwesomeApp 3. react-native install react-native-fit-image 4. react-native uninstall react-native-fit-image **Test plan NPM** 1. Publish to Sinopia: https://github.com/facebook/react-native/tree/master/react-native-cli 2. react-native init AwesomeAppNPM --npm 3. react-native install react-native-fit-image 4. react-native uninstall react-native-fit-image **output (yarn.lock)** ``` > react-native install react-native-fit-image yarn add v0.16.1 [1/4] 🔍 Resolving packages... [2/4] 🚚 Fetching packages... [3/4] 🔗 Linking dependencies... [4/4] 📃 Building fresh packages... success Saved lockfile. success Saved 1 new dependency └─ react-native-fit-image@1.4.6 ✨ Done in 4.11s. rnpm-install info Module react-native-fit-image has been successfully Closes facebook/react-native#11174 Differential Revision: D4441872 fbshipit-source-id: 05bd30e1ad14bdbe861259c88e1f18df77cfa54f

facebook-github-bot 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 Nov 28, 2016

mkonicek reviewed Nov 29, 2016

View reviewed changes

mkonicek removed the GH Review: review-needed label Nov 29, 2016

grabbou reviewed Nov 29, 2016

View reviewed changes

Kureev previously approved these changes Dec 2, 2016

View reviewed changes

n3tr mentioned this pull request Dec 3, 2016

WIP: add yarn support for package install #11280

Closed

olingern mentioned this pull request Dec 4, 2016

react-native install to use Yarn #11122

Closed

mkonicek added the GH Review: needs-revision label Dec 5, 2016

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Dec 8, 2016

Kureev approved these changes Dec 10, 2016

View reviewed changes

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jan 20, 2017

facebook-github-bot added Import Failed Import Started This pull request has been imported. This does not imply the PR has been approved. and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jan 20, 2017

facebook-github-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 2, 2017

n3tr added 8 commits February 2, 2017 16:46

Use yarn for install/uninstall CLI if available

a49e847

- Use yarn if available - Fix lint warning

use process.cwd() to get project dir

6b605c2

Missing License

777e072

Add PackageManager.js with add/remove func

5a7912a

- PackageManager.add(‘some-package’)

Make install/uninstall cli to use PackageManager

99782e1

missing semi-colons

4a209eb

make call to react-native link instead of rnpm

b73edc4

Split package argument by @ to get package name

f7c072b

- Support install some-package@version

n3tr force-pushed the yarn-install-cli branch from 705b989 to f7c072b Compare February 2, 2017 09:53

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 2, 2017

facebook-github-bot closed this in 1c249e4 Feb 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Yarn for Install/Uninstall CLI if available #11174

Use Yarn for Install/Uninstall CLI if available #11174

n3tr commented Nov 28, 2016 •

edited

Loading

facebook-github-bot commented Nov 28, 2016

mkonicek Nov 29, 2016

n3tr Nov 29, 2016 •

edited

Loading

n3tr Nov 30, 2016

mkonicek Dec 5, 2016

mkonicek Nov 29, 2016

mkonicek Nov 29, 2016 •

edited

Loading

n3tr Nov 29, 2016

mkonicek Dec 5, 2016 •

edited

Loading

mkonicek commented Nov 29, 2016 •

edited

Loading

grabbou Nov 29, 2016

n3tr Nov 30, 2016

Kureev Dec 1, 2016 •

edited

Loading

Kureev Dec 1, 2016

Kureev Dec 1, 2016

n3tr Dec 1, 2016 •

edited

Loading

Kureev Dec 1, 2016

n3tr Dec 1, 2016

Kureev Dec 2, 2016

Kureev Dec 2, 2016

mkonicek commented Dec 5, 2016

n3tr commented Dec 5, 2016 •

edited

Loading

Kureev commented Dec 8, 2016 •

edited

Loading

n3tr commented Dec 8, 2016

Kureev commented Dec 9, 2016

n3tr commented Dec 9, 2016

Kureev left a comment

facebook-github-bot commented Jan 20, 2017

Kureev commented Feb 2, 2017

n3tr commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

Use Yarn for Install/Uninstall CLI if available #11174

Use Yarn for Install/Uninstall CLI if available #11174

Conversation

n3tr commented Nov 28, 2016 • edited Loading

facebook-github-bot commented Nov 28, 2016

Choose a reason for hiding this comment

n3tr Nov 29, 2016 • edited Loading

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

mkonicek Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkonicek Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

mkonicek commented Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kureev Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3tr Dec 1, 2016 • edited Loading

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

mkonicek commented Dec 5, 2016

n3tr commented Dec 5, 2016 • edited Loading

Kureev commented Dec 8, 2016 • edited Loading

n3tr commented Dec 8, 2016

Kureev commented Dec 9, 2016

n3tr commented Dec 9, 2016

Kureev left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Jan 20, 2017

Kureev commented Feb 2, 2017

n3tr commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

facebook-github-bot commented Feb 2, 2017

n3tr commented Nov 28, 2016 •

edited

Loading

n3tr Nov 29, 2016 •

edited

Loading

mkonicek Nov 29, 2016 •

edited

Loading

mkonicek Dec 5, 2016 •

edited

Loading

mkonicek commented Nov 29, 2016 •

edited

Loading

Kureev Dec 1, 2016 •

edited

Loading

n3tr Dec 1, 2016 •

edited

Loading

n3tr commented Dec 5, 2016 •

edited

Loading

Kureev commented Dec 8, 2016 •

edited

Loading