Switch to npm5

[felixfbecker]

This should make installation time (and therefor builds) much faster and make lockfiles way more reliable, especially in regard to handling of optionalDependencies like fsevents.

I just ran scripts/npm install with npm5, which upgrades all the shrinkwrap files, then renamed then.
Additionally updated npm version in Travis and adapted gulpfiles.
I recommend merging this without squashing so the renames are recorded and not a huge delete+add.

[mention-bot]

@felixfbecker, thanks for your PR! By analyzing the history of the files in this pull request, we identified @joaomoreno and @bpasero to be potential reviewers.

[felixfbecker]

Install step on master: 350.53s
Install step on this branch: 143.84s

Total build time on master: 21min 39s
Total build time on this branch: 14min 45s

[joaomoreno]

Awesome numbers. Is the idea that people stop using shrinkwrap altogether?

Did you actually update to the latest versions of our runtime dependencies? That is something we wouldn't take very lightly as it's bitten us in the past - odd issues here and there. Can we somehow do this without actually updating our runtime dependencies versions?

[felixfbecker]

For projects, yes. shrinkwrap still serves a purpose for locking the dependency versions of published packages, e.g. if you install gulp and gulp had an npm-shrinkwrap.json, the dependency tree of gulp's dependencies would be locked. You generally don't want this. npm-shrinkwrap.json and package-lock.json have an identical schema, but the former is published to npm, the latter not. package-lock.json is the default for new projects.

I did not update any versions - npm migrates the schema automatically, and I then renamed all the files. All dependencies should still be locked to the same version (you can see this if you look at cc38d99). Also, devDependencies are now included by default.

[joaomoreno]

What happens if we leave the shrinkwrap files in the repo as well? If we remove them we force everyone to update to npm@5 at once, which can be done, yet is not ideal.

[felixfbecker]

I wouldn't keep them both, because then we risk them getting out of sync. It also wouldn't be of any use because the schema was upgraded and is not backwards-compatible (i.e. npm5 can upgrade npm4 shrinkwraps, but npm4 can't read upgraded npm5 shrinkwraps/package-locks)

[joaomoreno]

Exactly, so it needs to be a coordinated event across the whole dev team, internal and external build definitions and documentation.

[felixfbecker]

yep, just like on a Node version update.
As this PR is very prone to merge conflicts and a merge conflict fix means redoing the whole PR, it would be nice if this could get merged quickly - wdyt about adding a check to npm.sh that checks the npm version and prints something like "Please run npm -g npm to update to npm 5"? That would make the fix obvious and easy to any dev who's still running npm 4 :)

[felixfbecker]

I added a check to both npm.sh and npm.bat and tested on mac and Windows

$ scripts/npm.sh i
VS Code requires npm v5
Please run npm i -g npm to update

[joaomoreno]

@alexandrudima Pointed out another source of issues: our internal OSS tracking tool, which currently gets its input from npm-shrinkwrap.json files. It shouldn't be tough to upgrade though.

[joaomoreno]

@felixfbecker I'd put the npm checks in here instead: https://github.com/Microsoft/vscode/blob/master/build/npm/preinstall.js#L6

We use the preinstall.js file for catching issues when launching npm. It's usually only executed during install, but that should be fine. I just want to keep the batch scripts as small as possible.

[felixfbecker]

Done.

[felixfbecker]

@alexandrudima Pointed out another source of issues: our internal OSS tracking tool, which currently gets its input from npm-shrinkwrap.json files. It shouldn't be tough to upgrade though.

Depending on what properties it uses, it might not have to change at all

[felixfbecker]

Oh boy, shrinkwraps got updated in master...

[felixfbecker]

Rebased and re-migrated all changed shrinkwrap files.

[joaomoreno]

@felixfbecker No worries about keeping this up to date with master, I can take it from here. I plan on merging this next week, after endgame.

[felixfbecker]

Okay, cool. My process was (with knowing that no new npm-shrinkwrap was added):

• git rebase -i master
• edit the commit "Update to npm5"
• reset all files to HEAD
• run scripts/npm.sh install with npm 5 (twice to ensure it's stable)
• git add .; git commit --amend --no-edit; git rebase --continue

Because the rename is a separate commit this was easier than I thought.

[joaomoreno]

Spent the whole afternoon on this. It's not as trivial at it looks.

• npm 5.0.4 has issues, npm install fails with that version. So make sure you use 5.0.3.
• Some dependencies seem to be missing from the top level package-lock.json, especially the platform dependent, optional ones, like windows-mutex. Need to look into that.
• The package-lock.json inside build/lib/watch is broken, so one needs to delete it to make this work.

I'll pick it up tomorrow, but I might just trash this PR and start over. Sorry for that @felixfbecker.

On the plus side: I've made all our TFS build agents dependent on NVM, so we can pick any node version during the build. This was crucial, in order to be able to build master as well as the old release branches.

[felixfbecker]

Some dependencies seem to be missing from the top level package-lock.json, especially the platform dependent, optional ones, like windows-mutex. Need to look into that.

I think with shrinkwrap it only saved what you had installed, so if you run shrinkwrap on a mac, it would save fsevents, if you run it on a Windows machine, it would save windows-mutex - never optionalDependencies not installed. If you tried to install from a shrinkwrap that included platform-incompatible optionalDependencies, npm would error. I think npm 5 now saves all optional dependencies, but only installs those that can be installed.
But because I just renamed the files, the needed ones might not be in there.

Another problem I saw on this branch is that there seems to be some manual modifications in the npm-shrinkwrap.json to replace transitive dependencies with a fork - this seems to fail for npm5 with an EINTEGRITY error because the forked version's hash doesn't match the hash in the shrinkwrap (just a theory, could be different issue).

No problem if you start over, I'm happy if npm5 makes it into master in any way :)
I hope this PR still served a purpose as a proof of concept or draft.

[joaomoreno]

And as I suspected: npm i will always update package-lock.json with whatever it really installed. Given that fsevents won't install on Windows, running npm i on Windows (after locking all dependencies in macOS) updates the package-lock.json and removes it from there:

We can't really take this right now since we have optional dependencies which are platform specific. We'd have constant changes in these files all the time.

I see some activity going on in npm/npm#16839, which seems related, let's see if that fixes it.

I've created #30134 so we remember to do this eventually.