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.

Sign up for GitHub

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

Jump to bottom

feat(tracking): Third party NPM replacements #4321

Merged

hansl merged 1 commit into angular:master from sumitarora:feat-npm-replacements2

Feb 9, 2017

Contributor

sumitarora commented Feb 1, 2017

Check for Third party NPM replacements and make use of those if set by default
use ng set --global packageManager=yarn or ng set --global packageManager=cnpm to check

Fixes: #3886

googlebot added the cla: yes label

sumitarora added the state: WIP label

Contributor

Brocco commented Feb 1, 2017

Can you come up with a way to add some tests around this?

hansl suggested changes

View reviewed changes

Contributor

hansl left a comment

Good start! thanks a lot.

packages/angular-cli/commands/init.run.ts Outdated

                 let npmInstall: any;
                 if (!commandOptions.skipNpm) {
+                  const packageManager = CliConfig.fromProject().get('packageManager') || 'npm';

Contributor

hansl Feb 1, 2017

No need to || for values in the config, you should set a default value in the schema.json instead.

packages/angular-cli/lib/config/schema.json Outdated

@@ @@ -293,6 +293,9 @@ @@
                     },
                     "additionalProperties": false
                   },
+                  "packageManager": {
+                      "enum": [ "npm", "cnpm", "yarn" ]

Contributor

hansl Feb 1, 2017

There should be another value here, "default", which enables the check. People might want to use npm without having the warning shown up.

packages/angular-cli/utilities/check-package-manager.ts Outdated

+              }
+              export function checkYarnOrCNPM() {
+                return new Promise((resolve, reject) => {

Contributor

hansl Feb 1, 2017

No need to create a new promise (and it's kinda awkward). Just return the Promise.all().then().catch() directly.

packages/angular-cli/utilities/check-package-manager.ts Outdated

+              }
+              function checkYarn() {
+                return new Promise(function(resolve, reject) {

Contributor

hansl Feb 1, 2017

You can use Promise.denodeify for this (look up elsewhere in the code where we use it);

const exec = Promise.denodeify(fs.exec);

function checkYarn() {
  return exec('yarn --version')
    .then(() => true, () => false);
}

Much cleaner :)

packages/angular-cli/utilities/check-package-manager.ts Outdated

+                      }
+                      resolve();
+                    })
+                    .catch(resolve);

Contributor

hansl Feb 1, 2017

I don't think we should swallow errors.

In case you didn't know, using a catch on a Promise and not throwing an error will just swallow the rejection. So you can do something like:

Promise.resolve()
  .then(() => doSomething())  // this throws
  .catch(() => {});  // this will swallow the error and the promise returned here will be resolved properly.

Again though, if the code above generates an error I think we should propagate it back to the user.

packages/angular-cli/utilities/check-package-manager.ts Outdated

+                  Promise
+                    .all([checkYarn(), checkCNPM()])
+                    .then((data) => {
+                      if (packageManager === 'npm') {

Contributor

hansl Feb 1, 2017

If this checks is done by the function calling this, it would reduce the code duplication and make the overall structure easier to understand.

packages/angular-cli/utilities/check-package-manager.ts Outdated

+                return new Promise((resolve, reject) => {
+                  Promise
+                    .all([checkYarn(), checkCNPM()])
+                    .then((data) => {

Contributor

hansl Feb 1, 2017

You can use destructuring, like .then(([isYarnInstalled, isCnpmInstalled]) => { /* ... */ }) here, saving a few lines of code and making the general reading flow better.

packages/angular-cli/commands/init.run.ts Outdated

		@@ -1,7 +1,10 @@
		import * as chalk from 'chalk';
		import * as check from '../utilities/check-package-manager';

Contributor

hansl Feb 1, 2017

Normally unless there's a conflict you should import {functionName} from '...'.

sumitarora force-pushed the feat-npm-replacements2 branch 13 times, most recently from d105844 to 620c118 Compare

February 3, 2017 20:10

sumitarora removed the state: WIP label

sumitarora force-pushed the feat-npm-replacements2 branch 3 times, most recently from 3d7f5a0 to 23c7286 Compare

February 6, 2017 16:45

hansl suggested changes

View reviewed changes

Contributor

hansl left a comment

Few more nits. Getting close!

packages/@angular/cli/lib/config/schema.json Outdated

@@ @@ -292,6 +292,10 @@ @@
                     },
                     "additionalProperties": false
                   },
+                  "packageManager": {
+                      "enum": [ "npm", "cnpm", "yarn" ],

Contributor

hansl Feb 6, 2017

I would like the default to be a fourth value, so that people who forces it to npm don't get the message.

packages/@angular/cli/utilities/check-package-manager.ts Outdated

+                      if (packageManager === 'npm') {
+                        const [isYarnInstalled, isCNPMInstalled] = data;
+                        if (isYarnInstalled && isCNPMInstalled) {
+                          console.log(chalk.yellow('you can `ng set --global packageManager=yarn` ' +

Contributor

hansl Feb 6, 2017

Phrases should start with a capital letter and ends with a dot. Same for other logging messages.

sumitarora force-pushed the feat-npm-replacements2 branch 3 times, most recently from 9b87dd1 to 75b61e7 Compare

February 6, 2017 21:49

hansl suggested changes

View reviewed changes

packages/@angular/cli/lib/config/schema.json Outdated

@@ @@ -292,6 +292,10 @@ @@
                     },
                     "additionalProperties": false
                   },
+                  "packageManager": {
+                      "enum": [ "npm", "cnpm", "yarn", "default" ],
+                      "default": "npm"

Contributor

hansl Feb 7, 2017

default should be default.

Contributor Author

sumitarora Feb 7, 2017

@hansl I tried but then it always picks value as default and it fails install as there is nothing for default install. Am I missing how it should be done ?

packages/@angular/cli/utilities/check-package-manager.ts Outdated

+                      if (packageManager === 'npm') {
+                        const [isYarnInstalled, isCNPMInstalled] = data;
+                        if (isYarnInstalled && isCNPMInstalled) {
+                          console.log(chalk.yellow('You can `ng set --global packageManager=yarn` ' +

Contributor

hansl Feb 7, 2017

Operators go to the start of the next line.

hansl suggested changes

View reviewed changes

Contributor

hansl left a comment

More comments.

packages/@angular/cli/commands/init.run.ts Outdated

               const Promise = require('../ember-cli/lib/ext/promise');
               const SilentError = require('silent-error');
               const normalizeBlueprint = require('../ember-cli/lib/utilities/normalize-blueprint-option');
               const GitInit = require('../tasks/git-init');
+              const config = CliConfig.fromProject();

Contributor

hansl Feb 7, 2017

The project's config will never exist in new. You probably want fromGlobal()?

packages/@angular/cli/commands/init.run.ts Outdated

                 let npmInstall: any;
                 if (!commandOptions.skipNpm) {
+                  let packageManager = 'npm';

Contributor

hansl Feb 7, 2017

I don't think you need this here, since config.get(...) will give you the default value.

packages/@angular/cli/utilities/check-package-manager.ts Outdated

+                return Promise
+                    .all([checkYarn(), checkCNPM()])
+                    .then((data: Array<boolean>) => {
+                      if (packageManager === 'npm') {

Contributor

hansl Feb 7, 2017

So you're checking both yarn and cnpm EVEN if the user asked for a specific one? That's a lot of unnecessary work (and will slow down the bootstrapping time significantly).

Contributor Author

sumitarora Feb 7, 2017

How is user asking? If I have yarn/cnpm installed I just check for any and use whichever I found. Use just does ng new foo

Contributor

Brocco commented Feb 7, 2017

As part of this change, can you update the name of the option in new & init from skip-npm to skip-install as well as the documentation. If people are using yarn or cnpm then the name of the option no longer makes sense.

sumitarora force-pushed the feat-npm-replacements2 branch 2 times, most recently from 1e3c341 to 034c501 Compare

February 7, 2017 16:38

Brocco reviewed

View reviewed changes

packages/@angular/cli/tasks/npm-install.ts

    
                    ui.writeLine(chalk.green('Installing packages for tooling via npm.'));

                    exec('npm install',

                    ui.writeLine(chalk.green(`Installing packages for tooling via ${packageManager}.`));

                    exec(`${packageManager} install`,

Contributor

Brocco Feb 7, 2017

No need to change now, but if we do add support for another package manager that may not use install as the command, this code will need updated.

Contributor

Brocco commented Feb 7, 2017

Please add breaking change info to the PR message so that it's included in the changelog with the release.

You can find that info here: https://github.com/angular/angular-cli/blob/master/CONTRIBUTING.md#commit

sumitarora force-pushed the feat-npm-replacements2 branch 3 times, most recently from 9179c35 to 746f09b Compare

February 8, 2017 03:32

hansl approved these changes

View reviewed changes

Contributor

hansl commented Feb 8, 2017

@sumitarora you need to change the tests who are also using --skip-npm.

Contributor Author

sumitarora commented Feb 8, 2017

@hansl yeap doing so 👍

sumitarora force-pushed the feat-npm-replacements2 branch 4 times, most recently from deab013 to 6fe08e6 Compare

February 8, 2017 21:06

hansl reviewed

View reviewed changes

tests/e2e/tests/commands/new/check-yarn.ts Outdated

+                  .then(() =>  ng('set', '--global', 'packageManager=default'))
+                  .then(() =>  ng('new', 'foo'))
+                  .then((stdout) => {
+                    // assuming yarn is installed and checking for message with yarn

Contributor

hansl Feb 8, 2017

Comments should start with a capital letter and end with a dot.

hansl suggested changes

View reviewed changes

Contributor

hansl left a comment

One nit, but approved. Good work!

hansl approved these changes

View reviewed changes

sumitarora force-pushed the feat-npm-replacements2 branch 5 times, most recently from ddbc295 to d0dfed8 Compare

February 9, 2017 00:51


          feat(@angular/cli): Add options for third party package manager

e486bc2

BREAKING CHANGE: `--skip-npm` flag is now named `--skip-install`

sumitarora force-pushed the feat-npm-replacements2 branch from d0dfed8 to e486bc2 Compare

February 9, 2017 17:16

hansl merged commit d2849c7 into angular:master

sumitarora deleted the feat-npm-replacements2 branch

March 24, 2017 16:35

asnowwolf pushed a commit to asnowwolf/angular-cli that referenced this pull request


          feat(@angular/cli): Add options for third party package manager (angu…

2a2095f

…lar#4321)

BREAKING CHANGE: `--skip-npm` flag is now named `--skip-install`

angular-automatic-lock-bot bot commented Sep 11, 2019

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

angular-automatic-lock-bot bot locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet