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

[ENHANCEMENT] Use npm 3 #6306

Merged
merged 1 commit into from Oct 1, 2016

Conversation

Projects
None yet
6 participants
@dfreeman
Contributor

dfreeman commented Sep 30, 2016

I was prepared for this to be a lot more painful, but it turns out the only thing breaking the test suite with npm 3 was transitive dependency resolution in the cached .node_modules-tmp directory.

Since v3 flattens dependencies where possible, where we'd have .node_modules-tmp/pkg-a/node_modules/pkg-b before, now we get .node_modules-tmp/{pkg-a,pkg-b}. Node will only resolve a require from one package to its peer when they're both in a directory called "node_modules", so require('pkg-b') would fail from pkg-a.

The change here is just to nest the shared directory so that it can be called node_modules and then node's resolution process will do the right thing.

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Sep 30, 2016

Contributor

Have reviewed, LFantasticTM. I was worried we'd have to do another one of these.

Considering this will likely have some surprising and weird side effects for a few of our end users I'll write a little blurb for the changelog for the first time this is released (2.10.0-beta.1).

@ember-cli/core I'd love for everybody to weigh in on possible concerns for shifting to npm3 over the next day so that we make sure we address all the things in time for the 2.10 release.

@kategengler you come to mind in particular...

Contributor

nathanhammond commented Sep 30, 2016

Have reviewed, LFantasticTM. I was worried we'd have to do another one of these.

Considering this will likely have some surprising and weird side effects for a few of our end users I'll write a little blurb for the changelog for the first time this is released (2.10.0-beta.1).

@ember-cli/core I'd love for everybody to weigh in on possible concerns for shifting to npm3 over the next day so that we make sure we address all the things in time for the 2.10 release.

@kategengler you come to mind in particular...

@nathanhammond nathanhammond added this to the v2.10.0 milestone Sep 30, 2016

@kellyselden

This comment has been minimized.

Show comment
Hide comment
@kellyselden

kellyselden Sep 30, 2016

Member

talk about elegant... 💯

Member

kellyselden commented Sep 30, 2016

talk about elegant... 💯

@Turbo87

This comment has been minimized.

Show comment
Hide comment
@Turbo87

Turbo87 Sep 30, 2016

Member

I've just tried this and it seems to work well. The UI while installing has changed though since the NPM output comes through now and is no longer silent. Also while bower installing I was seeing some NPM output which was a bit confusing.

Member

Turbo87 commented Sep 30, 2016

I've just tried this and it seems to work well. The UI while installing has changed though since the NPM output comes through now and is no longer silent. Also while bower installing I was seeing some NPM output which was a bit confusing.

@kategengler

This comment has been minimized.

Show comment
Hide comment
@kategengler

kategengler Sep 30, 2016

Contributor

@nathanhammond Because of ember-try? Should be fine.

Contributor

kategengler commented Sep 30, 2016

@nathanhammond Because of ember-try? Should be fine.

@dfreeman

This comment has been minimized.

Show comment
Hide comment
@dfreeman

dfreeman Sep 30, 2016

Contributor

@Turbo87 I can look into silencing the npm output during installation, if that's desirable.

I actually really liked having the details about how far along the installation process was (much better than npm@2's / spinner), but I'm not sure whether there are still perf issues associated with having the progress displayed.

Based on npm/npm#11283, it looks like a nonzero but < 5% perf impact, which is consistent with my totally-unscientific testing:

ember new app-with-progress     51.48s user 17.40s system 63% cpu 1:48.94 total
ember new app-without-progress  48.81s user 16.33s system 62% cpu 1:44.65 total
Contributor

dfreeman commented Sep 30, 2016

@Turbo87 I can look into silencing the npm output during installation, if that's desirable.

I actually really liked having the details about how far along the installation process was (much better than npm@2's / spinner), but I'm not sure whether there are still perf issues associated with having the progress displayed.

Based on npm/npm#11283, it looks like a nonzero but < 5% perf impact, which is consistent with my totally-unscientific testing:

ember new app-with-progress     51.48s user 17.40s system 63% cpu 1:48.94 total
ember new app-without-progress  48.81s user 16.33s system 62% cpu 1:44.65 total
@Turbo87

This comment has been minimized.

Show comment
Hide comment
@Turbo87

Turbo87 Sep 30, 2016

Member

yeah, I'm fine with having the output, we should just make sure not to leak npm output into the bower install step

Member

Turbo87 commented Sep 30, 2016

yeah, I'm fine with having the output, we should just make sure not to leak npm output into the bower install step

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Oct 1, 2016

Contributor

yeah, I'm fine with having the output, we should just make sure not to leak npm output into the bower install step
Add your review

they should happen in serial, so there should be no leak.

Contributor

stefanpenner commented Oct 1, 2016

yeah, I'm fine with having the output, we should just make sure not to leak npm output into the bower install step
Add your review

they should happen in serial, so there should be no leak.

@stefanpenner stefanpenner merged commit 63cf075 into ember-cli:master Oct 1, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 92.16%
Details
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Oct 1, 2016

Contributor

Since v3 flattens dependencies where possible, where we'd have .node_modules-tmp/pkg-a/node_modules/pkg-b before, now we get .node_modules-tmp/{pkg-a,pkg-b}. Node will only resolve a require from one package to its peer when they're both in a directory called "node_modules", so require('pkg-b') would fail from pkg-a.

thanks for digging into this, the upgrade issues left me perplexed. I am glad you were able to get to the bottom of it.

Contributor

stefanpenner commented Oct 1, 2016

Since v3 flattens dependencies where possible, where we'd have .node_modules-tmp/pkg-a/node_modules/pkg-b before, now we get .node_modules-tmp/{pkg-a,pkg-b}. Node will only resolve a require from one package to its peer when they're both in a directory called "node_modules", so require('pkg-b') would fail from pkg-a.

thanks for digging into this, the upgrade issues left me perplexed. I am glad you were able to get to the bottom of it.

@Turbo87

This comment has been minimized.

Show comment
Hide comment
@Turbo87

Turbo87 Oct 1, 2016

Member

they should happen in serial, so there should be no leak.

in theory yes, but that's what I saw happening with the console output...

Member

Turbo87 commented Oct 1, 2016

they should happen in serial, so there should be no leak.

in theory yes, but that's what I saw happening with the console output...

@dfreeman

This comment has been minimized.

Show comment
Hide comment
@dfreeman

dfreeman Oct 1, 2016

Contributor

@Turbo87 I looked into it a little further — npmlog queues up output (including progress bar updates) and then periodically flushes them pretty much on autopilot, and it looks like the final flush happens after the install command invokes the given callback.

This seems like a bug in npm to me, but I can imagine workarounds on this end. Thoughts?

Contributor

dfreeman commented Oct 1, 2016

@Turbo87 I looked into it a little further — npmlog queues up output (including progress bar updates) and then periodically flushes them pretty much on autopilot, and it looks like the final flush happens after the install command invokes the given callback.

This seems like a bug in npm to me, but I can imagine workarounds on this end. Thoughts?

@dfreeman dfreeman deleted the dfreeman:npm-3 branch Oct 1, 2016

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Oct 1, 2016

Contributor

This seems like a bug in npm to me, but I can imagine workarounds on this end. Thoughts?

yup sounds reasonable.

Contributor

stefanpenner commented Oct 1, 2016

This seems like a bug in npm to me, but I can imagine workarounds on this end. Thoughts?

yup sounds reasonable.

@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Oct 3, 2016

Contributor

@dfreeman I think I'd file a bug at npmlog instead of into the 2343 open issues at NPM. Would you be willing to do that?

I don't personally care enough on our side to make this perfect since it works just fine but I'd like to at least catalog it for them.

Contributor

nathanhammond commented Oct 3, 2016

@dfreeman I think I'd file a bug at npmlog instead of into the 2343 open issues at NPM. Would you be willing to do that?

I don't personally care enough on our side to make this perfect since it works just fine but I'd like to at least catalog it for them.

@dfreeman

This comment has been minimized.

Show comment
Hide comment
@dfreeman

dfreeman Oct 4, 2016

Contributor

@nathanhammond It's not totally clear to me where the fix will need to happen — I haven't fully wrapped my head around the interplay between npmlog, gauge, are-we-there-yet and the special sauce in npm's install command.

I'll file on npmlog since that appears to be lower-traffic and see what direction comes out of that.

Contributor

dfreeman commented Oct 4, 2016

@nathanhammond It's not totally clear to me where the fix will need to happen — I haven't fully wrapped my head around the interplay between npmlog, gauge, are-we-there-yet and the special sauce in npm's install command.

I'll file on npmlog since that appears to be lower-traffic and see what direction comes out of that.

@dfreeman

This comment has been minimized.

Show comment
Hide comment
@dfreeman

dfreeman Oct 5, 2016

Contributor

Following back up on the logging leakage, it looks like npm essentially just always has a progress meter going while it's running (it gets turned on when we call load()), and then they just occasionally turn it off for short periods when emitting other text.

Since the only officially-supported way to invoke the install command is via the npm CLI, they just let the process exit clean up the running progress bar rather than explicitly disabling it.

I don't know the full history behind why npm is executed the way it is from this end, but if execing it is a nonstarter, two options I can think of off the top of my head would be to either:

  • just pass progress: false to the command and accept losing the nice output
  • play some resolution tricks to get ahold of npmlog and call disableProgress on it manually after the command completes
Contributor

dfreeman commented Oct 5, 2016

Following back up on the logging leakage, it looks like npm essentially just always has a progress meter going while it's running (it gets turned on when we call load()), and then they just occasionally turn it off for short periods when emitting other text.

Since the only officially-supported way to invoke the install command is via the npm CLI, they just let the process exit clean up the running progress bar rather than explicitly disabling it.

I don't know the full history behind why npm is executed the way it is from this end, but if execing it is a nonstarter, two options I can think of off the top of my head would be to either:

  • just pass progress: false to the command and accept losing the nice output
  • play some resolution tricks to get ahold of npmlog and call disableProgress on it manually after the command completes
@nathanhammond

This comment has been minimized.

Show comment
Hide comment
@nathanhammond

nathanhammond Oct 6, 2016

Contributor

The "only use npm via CLI" guidance is more recent than our adoption of that pattern. Since we were trying to create our own consistent environment (this came out of lots of broken npm releases) we elected to control npm. I also feel like that recommendation is ill-conceived considering the npm@3 recommendation that the peerDependencies key be managed by the application that is consuming them.

Contributor

nathanhammond commented Oct 6, 2016

The "only use npm via CLI" guidance is more recent than our adoption of that pattern. Since we were trying to create our own consistent environment (this came out of lots of broken npm releases) we elected to control npm. I also feel like that recommendation is ill-conceived considering the npm@3 recommendation that the peerDependencies key be managed by the application that is consuming them.

@nathanhammond nathanhammond referenced this pull request Oct 13, 2016

Closed

bundle npm v3 #5082

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment