upgrade npm to 3.7.1

[iarna]

This contains these releases:

https://github.com/npm/npm/releases/tag/v3.7.0
https://github.com/npm/npm/releases/tag/v3.7.1

Notable changes are:

• Performance improvements
• Support for git submodules

r: @Fishrock123
r: @zkat

[MylesBorins]

I'm getting some weirdness over here

npm ERR! code MODULE_NOT_FOUND

npm ERR! Cannot find module 'internal/util'

Eeek!

It seems like the tests on v5.x work as expected as do the tests at 76cb81b when it was last upgraded.
The previous commit with v3.6.0 is experiencing the same weirdness, so I think this is on our end.
Bisecting

[MylesBorins]

Looks like it was 1124de2 which landed about 4 hours ago and it is semver major

¯_(ツ)_/¯

PR for reference #4525

/cc @thefourtheye @jasnell @ChALkeR @bnoordhuis

It looks like there was a search against the ecosystem, but perhaps not done against npm itself

#4530 (comment)

edit:

I just have to note how crazy it is the a deprecation that was supposed to happen in 2010 finally lands an hour before a release that it breaks... I'm without words

[ChALkeR]

@thealphanerd, it should not fail, the API change probably had nothing to do with it.

Most probably something is re-executing source code of the fs module, monkey-patching it. Will take a look later today.

[MylesBorins]

@ChALkeR my thoughts exactly. Seemed very odd that this would break it. npm is effectively broken on master right now

[ChALkeR]

See:
#1898
#2025
#2026
#1941 (comment)

[ChALkeR]

@thealphanerd 1124de2 introduced require('internal/util'), which is not requirable from the external code (i.e. not from the Node.js core), so anything that wraps fs module code, monkey-patches it and re-executes it (as graceful-fs was doing a while ago) is broken by that.

[MylesBorins]

@ChALkeR should we be doing a try catch on that require?

edit: perhaps we can move this discussion to #4525

[ChALkeR]

@thealphanerd Why? I think we decided against supporting monkey-patching core modules in that way. Some module required by npm should be fixed.

Could you give me a trace, please?

[MylesBorins]

I was unaware of not supporting monkey-patching, but that is reasonable.

That being said This version of npm has some pretty important performance improvements. Do you think it makes sense to temporarily revert the offending change until the next release of npm which can perhaps fix this?

[ChALkeR]

@thealphanerd See the issues and PRs I linked above in #5097 (comment) for history on this.

[thefourtheye]

Okay. Actual problem is one of the npm's dependencies are still using older version of cmd-shim, which uses older, monkey-patched version of graceful-fs

Excerpt from npm-debug.log file generated when I ran make test-npm locally

258 verbose stack Error: Cannot find module 'internal/util'
258 verbose stack     at Function.Module._resolveFilename (module.js:339:15)
258 verbose stack     at Function.Module._load (module.js:290:25)
258 verbose stack     at Module.require (module.js:367:17)
258 verbose stack     at require (internal/module.js:16:19)
258 verbose stack     at evalmachine.<anonymous>:38:26
258 verbose stack     at Object.<anonymous> (/home/thefourtheye/git/io.js/test-npm/node_modules/cmd-shim/node_modules/graceful-fs/fs.js:11:1)
258 verbose stack     at Module._compile (module.js:417:34)
258 verbose stack     at Object.Module._extensions..js (module.js:426:10)
258 verbose stack     at Module.load (module.js:357:32)
258 verbose stack     at Function.Module._load (module.js:314:12)

---

Update:

On further investigation, npm itself is using cmd-shim@~2.0.1 and since there was no new release after merging npm/cmd-shim#14, npm still uses the monkey-patched graceful-fs (indirectly) and that is why this is failing. Because we don't allow userland modules to access modules in lib/internal and the older monkey-patched version of graceful-fs is trying to require internal/util. Probable fix is to have cmd-shim release a new version and then to have npm use the latest version of it.

[ChALkeR]

/cc @nodejs/ctc
It seems we have #1898 again. Some historic discussion in #2026.

[MylesBorins]

edit:
removed as I was totally off base, thanks @ChALkeR

[MylesBorins]

/cc @isaacs

[ChALkeR]

@thealphanerd, it doesn't.

[thefourtheye]

Okay, updated the comment #5097 (comment) with some findings.

[ChALkeR]

@nodejs/ctc My opinion here is that we should in fact fix 1124de2 on the Node.js side for now, but add an explicit warning there, for at least one major version (v6.x), so that everyone who monkey-patches fs in that way (e.g uses an old version of the graceful-fs module) will be notified of the problem and will get off it, because sudden breakage with a bogus error message could be confusing.

This does not cancel the fact that the problem should be fixed on cmd-shim/npm side, of course.

I will make a PR for that when I'll get home.

[Fishrock123]

Can we just remove lower versions of graceful-fs from the dep tree?

This was fixed ages ago in graceful-fs and I am not particularly sympathetic to it blatantly abusing the runtime code.

[ChALkeR]

@Fishrock123 That has to be done, of course, I'm not suggesting leaving an old version of graceful-fs hanging there.

[iarna]

As soon as the updated cmd-shim is published I'll add a commit to this PR that updates it. (It'll be two weeks before it shows up in the version of npm being upstreamed directly.)

@Fishrock123 We can't just remove the deeper one's from the dep tree, w/o patching cmd-shim or we'd be shipping a version of npm that's marked as invalid when you do an npm ls -g.

[ChALkeR]

Proposed solution:

1. Fix this on npm side.
2. Merge something like fs: add a temporary fix for re-evaluation support #5102 to give users a grace period in 6.x.

[Fishrock123]

@ChALkeR seems like an ok plan

[MylesBorins]

I made a branch that reverts 1124de2 and applies b5362b5

I then used that build of node to run citgm.

https://ci.nodejs.org/job/thealphanerd-smoker/59/

edit: all failures are unrelated

[MylesBorins]

closing in for #5211