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

Add package-manager option #618

Merged
merged 2 commits into from
Apr 30, 2017
Merged

Conversation

hrkt
Copy link
Contributor

@hrkt hrkt commented Mar 12, 2017

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • [?] The changes are appropriately documented (if applicable).
  • [?] The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

This PR introduces "package-manager" option.
It is a replacement of #614, and it tries to implement the spec defined in #515.

It can parse 'nom', 'yarn' as package-manager option.
When other package manager is specified, a warning message is shown.

This PR does not include the code below.

  1. modification of usage.txt
  2. unit test

Thanks in advance for reading this message.

Fixes #518.
Addresses #515.

@malept malept added docs-needed 📝 Pull request needs documentation needs work 🚧 Feature request needs to be fleshed out before maintainers can take action on it tests-needed Pull request needs tests labels Mar 13, 2017
Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

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

In addition to your notes about what's missing:

  • it should have cnpm support (which, as I understand it, is the exact same interface as npm)
  • there is one more reference to NPM in the tests that needs to be addressed.

common.js Outdated
@@ -88,6 +88,18 @@ function asarApp (appPath, asarOptions, cb) {
})
}

function pruneModules (opts, appPath, cb) {
var pmName = opts['package-manager']
Copy link
Member

Choose a reason for hiding this comment

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

per the spec:

It should be called packageManager in the API

common.js Outdated
} else {
// defaults to npm
child.exec('npm prune --production', { cwd: appPath }, cb)
}
Copy link
Member

Choose a reason for hiding this comment

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

There's a better way to structure this:

opts.packageManager ||= 'npm'

switch (packageManager) {
  case 'npm':
    // do stuff
  case 'yarn':
    // do stuff
  default:
    throw error;
}

@@ -293,8 +305,7 @@ module.exports = {
// appPath is predictable (e.g. before .app is renamed for mac)
if (opts.prune || opts.prune === undefined) {
operations.push(function (cb) {
debug('Running npm prune --production')
Copy link
Member

Choose a reason for hiding this comment

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

This should come back in some form or another. We need to be able to see when the prune operation occurs and with what package manager. It's pretty helpful for debugging user problems.

@hrkt
Copy link
Contributor Author

hrkt commented Mar 13, 2017

Thank you, @malept for the review. I'll fix it in this week.

@malept
Copy link
Member

malept commented Mar 13, 2017

For tests, I think what I'd like to do is have Travis/AppVeyor download Yarn/cnpm and make sure they work correctly with pruning.

The test reference to NPM should probably be configurable via an environment variable.

@malept malept force-pushed the add-package-manager-option branch 3 times, most recently from f31a75e to cb36641 Compare April 24, 2017 01:35
@malept
Copy link
Member

malept commented Apr 24, 2017

I've refactored this PR so that it also works with cnpm, and added docs/tests.

The test reference to NPM should probably be configurable via an environment variable.

I decided against this. I'm OK with having people working on tests needing to have NPM installed.

@malept malept force-pushed the add-package-manager-option branch from cb36641 to 5245040 Compare April 25, 2017 16:03
@malept
Copy link
Member

malept commented Apr 25, 2017

Need to fix calling cnpm on Windows (per CI failure) and also add a NEWS entry.

@malept malept force-pushed the add-package-manager-option branch from 978ec79 to 00689c2 Compare April 28, 2017 03:53
@malept
Copy link
Member

malept commented Apr 28, 2017

I'm no longer calling cnpm on Windows (see: #515 (comment)). I just need to squash my commits, and I think I'll merge.

@malept malept force-pushed the add-package-manager-option branch from 00689c2 to 4fd4be3 Compare April 29, 2017 00:54
@malept malept merged commit 4f17587 into electron:master Apr 30, 2017
@hrkt hrkt deleted the add-package-manager-option branch April 30, 2017 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-needed 📝 Pull request needs documentation needs work 🚧 Feature request needs to be fleshed out before maintainers can take action on it tests-needed Pull request needs tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants