Skip to content

Adds support for npm to polymer install.#145

Merged
bicknellr merged 9 commits intomasterfrom
install-npm
Apr 17, 2018
Merged

Adds support for npm to polymer install.#145
bicknellr merged 9 commits intomasterfrom
install-npm

Conversation

@bicknellr
Copy link
Member

@bicknellr bicknellr commented Apr 14, 2018

If the --npm flag is passed or npm: true is set in polymer.json, polymer install will run npm install instead of installing dependencies from Bower. (fixes #118)

@bicknellr bicknellr requested a review from rictic April 14, 2018 00:05
require('../install/install').install as typeof installTypeOnly;

// Use `npm` from the config, if available and not passed as a CLI arg.
if (options.npm === undefined && config.npm !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should really do this in a unified place

import StandardRenderer = require('bower/lib/renderers/StandardRenderer');
import BowerProject = require('bower/lib/core/Project');

const exec = util.promisify(child_process.exec);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is sadly not available in node 6, we just aren't running node 6 integration tests currently (see #136)


export async function install(options?: Options): Promise<void> {
if (options && options.npm) {
await npmInstall();
Copy link
Contributor

Choose a reason for hiding this comment

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

Equivalent and simpler: return npmInstall()

Copy link
Contributor

@rictic rictic left a comment

Choose a reason for hiding this comment

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

[marking requested changes]

child_process.exec('npm install', {cwd: process.cwd()}, (error, stdout, stderr) => {
error ? reject(error) : resolve([stdout, stderr]);
});
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

'the package name is read from package.json',
description: 'Sets npm mode: dependencies are installed from npm, ' +
'component directory is "node_modules" and the package name is read ' +
'from package.json',
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm starting to think that the --npm flag / config option description should be made more generic because it has to match across all commands ("Sets npm mode.") and each command's help description might be a better place to put specific information about how it reacts to the flag.

Copy link
Contributor

@rictic rictic Apr 17, 2018

Choose a reason for hiding this comment

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

Yeah, I think we need to do some unification to the way we handle options/config in the CLI

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[cli] Should polymer install --npm run npm install?

3 participants