Support NPM5

[joaomoreno]

More details on why this still isn't supported in #29576

[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.

[felixfbecker]

@joaomoreno npm 5.1 was just released and has fixes that might be relevant:

• This is a big PR that fixes a variety of issues when installing from package locks. If you were previously having issues with missing dependencies or unwanted removals, this might have fixed it:
  • It introduces a new package-lock.json field, called requires, which tracks which modules a given module requires.
  • It fixes Update Seti-icons to match Seti-UI #16839 which was caused by not having this information available, particularly when git dependencies were involved.
  • It fixes "Toggle Activity Bar Visibility" in Zen Mode Does Nothing #16866, allowing the package.json to trump the package-lock.json.
    npm ls now loads the shrinkwrap, which opens the door to showing a full tree of dependencies even when nothing is yet installed. (It doesn't do that yet though.)

https://github.com/npm/npm/releases/tag/v5.1.0

also related:

Regarding cross platformness, I'll make sure it's clearer, but yes, ALL optional dependencies, ALL dev dependencies should be in lock/shrinkwrap files even if you installed with --no-optional and --only=production.

npm/npm#16441 (comment)

[joaomoreno]

Just in time for a lazy friday afternoon! 🍺

[joaomoreno]

After rerunning ./scripts/npm.sh i a few times on macOS until it stabilised, I noticed that the optional dependencies still don't show up in the package-lock.json files.

Still, I pushed it to the branch. Then, jumped to Windows, and now it somehow fails to compile v8-profiler, no idea why:

joao@JOAOMORENO46E4 C:\Users\joao\Work\vscode
> .\scripts\npm.bat i

> [email protected] preinstall C:\Users\joao\Work\vscode
> node build/npm/preinstall.js

npm WARN read-shrinkwrap This version of npm is compatible with lockfileVersion@1, but npm-shrinkwrap.json was generated for lockfileVersion@0. I'll try to do my best with it!
npm WARN deprecated [email protected]: Use uuid module instead
npm WARN deprecated [email protected]: to-iso-string has been deprecated, use @segment/to-iso-string instead.
npm WARN deprecated [email protected]: Jade has been renamed to pug, please install the latest version of pug instead of jade

> [email protected] preinstall C:\Users\joao\Work\vscode\node_modules\v8-profiler
> node -e 'process.exit(0)'


> [email protected] preinstall C:\Users\joao\Work\vscode\node_modules\typechecker
> node ./cyclic.js


> [email protected] install C:\Users\joao\Work\vscode\node_modules\windows-mutex
> node-gyp rebuild


joao@JOAOMORENO46E4 C:\Users\joao\Work\vscode\node_modules\windows-mutex
> if not defined npm_config_node_gyp (node "C:\Users\joao\AppData\Roaming\nvm\v7.10.0\node_modules\npm\bin\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" rebuild )  else (node "" rebuild )
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  main.cc
  mutex.cc
  win_delay_load_hook.cc
..\src\mutex.cc(70): warning C4996: 'v8::Function::NewInstance': was declared deprecated [C:\Users\joao\Work\vscode\node_modul
es\windows-mutex\build\CreateMutex.vcxproj]
  C:\Users\joao\.node-gyp\iojs-1.6.6\deps\v8\include\v8.h(3644): note: see declaration of 'v8::Function::NewInstance'
     Creating library C:\Users\joao\Work\vscode\node_modules\windows-mutex\build\Release\CreateMutex.lib and object C:\Users\j
  oao\Work\vscode\node_modules\windows-mutex\build\Release\CreateMutex.exp
  Generating code
  Finished generating code
  CreateMutex.vcxproj -> C:\Users\joao\Work\vscode\node_modules\windows-mutex\build\Release\\CreateMutex.node
  CreateMutex.vcxproj -> C:\Users\joao\Work\vscode\node_modules\windows-mutex\build\Release\CreateMutex.pdb (Full PDB)

> [email protected] install C:\Users\joao\Work\vscode\node_modules\windows-foreground-love
> node-gyp rebuild


joao@JOAOMORENO46E4 C:\Users\joao\Work\vscode\node_modules\windows-foreground-love
> if not defined npm_config_node_gyp (node "C:\Users\joao\AppData\Roaming\nvm\v7.10.0\node_modules\npm\bin\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" rebuild )  else (node "" rebuild )
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  foreground-love.cc
  win_delay_load_hook.cc
     Creating library C:\Users\joao\Work\vscode\node_modules\windows-foreground-love\build\Release\foreground-love.lib and obj
  ect C:\Users\joao\Work\vscode\node_modules\windows-foreground-love\build\Release\foreground-love.exp
  Generating code
  Finished generating code
  foreground-love.vcxproj -> C:\Users\joao\Work\vscode\node_modules\windows-foreground-love\build\Release\\foreground-love.nod
  e
  foreground-love.vcxproj -> C:\Users\joao\Work\vscode\node_modules\windows-foreground-love\build\Release\foreground-love.pdb
  (Full PDB)

> [email protected] install C:\Users\joao\Work\vscode\node_modules\v8-profiler
> node-pre-gyp install --fallback-to-build

node-pre-gyp ERR! Tried to download(404): https://node-inspector.s3.amazonaws.com/profiler/v5.6.5/electron-v1.6-win32-x64.tar.gz
node-pre-gyp ERR! Pre-built binaries not found for [email protected] and [email protected] (electron-v1.6 ABI) (falling back to source compile with node-gyp)

joao@JOAOMORENO46E4 C:\Users\joao\Work\vscode\node_modules\v8-profiler
> if not defined npm_config_node_gyp (node "C:\Users\joao\AppData\Roaming\nvm\v7.10.0\node_modules\npm\bin\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" clean )  else (node "" clean )

joao@JOAOMORENO46E4 C:\Users\joao\Work\vscode\node_modules\v8-profiler
> if not defined npm_config_node_gyp (node "C:\Users\joao\AppData\Roaming\nvm\v7.10.0\node_modules\npm\bin\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" configure --fallback-to-build --module=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64\profiler.node --module_name=profiler --module_path=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64 )  else (node "" configure --fallback-to-build --module=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64\profiler.node --module_name=profiler --module_path=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64 )

joao@JOAOMORENO46E4 C:\Users\joao\Work\vscode\node_modules\v8-profiler
> if not defined npm_config_node_gyp (node "C:\Users\joao\AppData\Roaming\nvm\v7.10.0\node_modules\npm\bin\node-gyp-bin\\..\..\node_modules\node-gyp\bin\node-gyp.js" build --fallback-to-build --module=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64\profiler.node --module_name=profiler --module_path=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64 )  else (node "" build --fallback-to-build --module=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64\profiler.node --module_name=profiler --module_path=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64 )
Building the projects in this solution one at a time. To enable parallel build, please add the "/m" switch.
  profiler.cc
  cpu_profiler.cc
  cpu_profile.cc
  cpu_profile_node.cc
c:\users\joao\work\vscode\node_modules\nan\nan_new.h(208): warning C4244: 'argument': conversion from 'int64_t' to 'double', p
ossible loss of data (compiling source file ..\src\cpu_profile.cc) [C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\p
rofiler.vcxproj]
  ..\src\cpu_profile.cc(82): note: see reference to function template instantiation 'v8::Local<v8::Number> Nan::New<v8::Number
  ,int64_t>(A0)' being compiled
          with
          [
              A0=int64_t
          ]
  heap_profiler.cc
  heap_snapshot.cc
  heap_output_stream.cc
  heap_graph_node.cc
..\src\heap_snapshot.cc(46): error C2039: 'GetHiddenValue': is not a member of 'v8::Object' [C:\Users\joao\Work\vscode\node_mo
dules\v8-profiler\build\profiler.vcxproj]
  c:\users\joao\.node-gyp\iojs-1.6.6\deps\v8\include\v8.h(2881): note: see declaration of 'v8::Object'
..\src\heap_snapshot.cc(50): error C2039: 'SetHiddenValue': is not a member of 'v8::Object' [C:\Users\joao\Work\vscode\node_mo
dules\v8-profiler\build\profiler.vcxproj]
  c:\users\joao\.node-gyp\iojs-1.6.6\deps\v8\include\v8.h(2881): note: see declaration of 'v8::Object'
..\src\heap_snapshot.cc(101): warning C4244: 'argument': conversion from 'int64_t' to 'uint32_t', possible loss of data [C:\Us
ers\joao\Work\vscode\node_modules\v8-profiler\build\profiler.vcxproj]
..\src\heap_graph_node.cc(69): warning C4996: 'v8::Object::Has': was declared deprecated [C:\Users\joao\Work\vscode\node_modul
es\v8-profiler\build\profiler.vcxproj]
  c:\users\joao\.node-gyp\iojs-1.6.6\deps\v8\include\v8.h(2998): note: see declaration of 'v8::Object::Has'
  heap_graph_edge.cc
c:\users\joao\work\vscode\node_modules\nan\nan_new.h(208): warning C4244: 'argument': conversion from 'std::size_t' to 'double
', possible loss of data (compiling source file ..\src\heap_graph_node.cc) [C:\Users\joao\Work\vscode\node_modules\v8-profiler
\build\profiler.vcxproj]
  ..\src\heap_graph_node.cc(126): note: see reference to function template instantiation 'v8::Local<v8::Number> Nan::New<v8::N
  umber,std::size_t>(std::A0)' being compiled
          with
          [
              A0=std::size_t
          ]
  win_delay_load_hook.cc
gyp ERR! build error
gyp ERR! stack Error: `C:\Program Files (x86)\MSBuild\14.0\bin\msbuild.exe` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onExit (C:\Users\joao\AppData\Roaming\nvm\v7.10.0\node_modules\npm\node_modules\node-gyp\lib\build.js:258:23)
gyp ERR! stack     at emitTwo (events.js:106:13)
gyp ERR! stack     at ChildProcess.emit (events.js:194:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:215:12)
gyp ERR! System Windows_NT 10.0.15063
gyp ERR! command "C:\\Program Files\\nodejs\\node.exe" "C:\\Users\\joao\\AppData\\Roaming\\nvm\\v7.10.0\\node_modules\\npm\\node_modules\\node-gyp\\bin\\node-gyp.js" "build" "--fallback-to-build" "--module=C:\\Users\\joao\\Work\\vscode\\node_modules\\v8-profiler\\build\\profiler\\v5.6.5\\electron-v1.6-win32-x64\\profiler.node" "--module_name=profiler" "--module_path=C:\\Users\\joao\\Work\\vscode\\node_modules\\v8-profiler\\build\\profiler\\v5.6.5\\electron-v1.6-win32-x64"
gyp ERR! cwd C:\Users\joao\Work\vscode\node_modules\v8-profiler
gyp ERR! node -v v7.10.0
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok
node-pre-gyp ERR! build error
node-pre-gyp ERR! stack Error: Failed to execute 'node-gyp.cmd build --fallback-to-build --module=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64\profiler.node --module_name=profiler --module_path=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64' (1)
node-pre-gyp ERR! stack     at ChildProcess.<anonymous> (C:\Users\joao\Work\vscode\node_modules\node-pre-gyp\lib\util\compile.js:83:29)
node-pre-gyp ERR! stack     at emitTwo (events.js:106:13)
node-pre-gyp ERR! stack     at ChildProcess.emit (events.js:194:7)
node-pre-gyp ERR! stack     at maybeClose (internal/child_process.js:899:16)
node-pre-gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:226:5)
node-pre-gyp ERR! System Windows_NT 10.0.15063
node-pre-gyp ERR! command "C:\\Program Files\\nodejs\\node.exe" "C:\\Users\\joao\\Work\\vscode\\node_modules\\node-pre-gyp\\bin\\node-pre-gyp" "install" "--fallback-to-build"
node-pre-gyp ERR! cwd C:\Users\joao\Work\vscode\node_modules\v8-profiler
node-pre-gyp ERR! node -v v7.10.0
node-pre-gyp ERR! node-pre-gyp -v v0.6.36
node-pre-gyp ERR! not ok
Failed to execute 'node-gyp.cmd build --fallback-to-build --module=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64\profiler.node --module_name=profiler --module_path=C:\Users\joao\Work\vscode\node_modules\v8-profiler\build\profiler\v5.6.5\electron-v1.6-win32-x64' (1)
npm WARN optional SKIPPING OPTIONAL DEPENDENCY: [email protected] (node_modules\fsevents):
npm WARN notsup SKIPPING OPTIONAL DEPENDENCY: Unsupported platform for [email protected]: wanted {"os":"darwin","arch":"any"} (current: {"os":"win32","arch":"x64"})

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! [email protected] install: `node-pre-gyp install --fallback-to-build`
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] install script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\joao\.npm-electron\_logs\2017-07-07T14_33_10_633Z-debug.log

[felixfbecker]

Just to make sure the cause is not broken old shrinkwraps or broken migration code, does the same error happen if you delete the shrinkwraps prior to installing?

This feels like it might be worth to open an issue for at npm

[joaomoreno]

This was already without any shrinkwrap file around, simply having the package-lock.json files.

https://github.com/Microsoft/vscode/tree/joao/npm5

But now that you mention this, maybe I should try to migrate the shrinkwraps from scratch on 5.1

[joaomoreno]

Hm... just got the exact same error in macOS. Welcome to the Twilight Zone.

[ivanz]

I've managed to get npm install to work with NPM 5.3 with the changes in #30759 . That fixes the build error that @joaomoreno mentions, which I hit as well.

[ivanz]

Relevant issue from NPM for the optional dependency stuff: npm/npm#17722 . Can confirm that fsevents still gets removed on Windows from the lock file with npm 5.3

[Stanzilla]

Time to consider yarn?

[felixfbecker]

@Stanzilla I've personally met more bugs with yarn than with npm

[felixfbecker]

@joaomoreno coming back to this as I think it is major factor the very long build times and installation time especially with all the extensions.
From what I understand, the blocking issue is that npm install now updates the package-lock.json file by removing the not-installed platform-specific dependencies, whereas npm 4 only did so on npm shrinkwrap. So it is not that npm 5 is less smart than npm 4, it's just that the command was explicit before and you would then fix the file manually, whereas now it always gets updated on install, correct?

npm has an option to not touch the lockfile during installation:

The --no-package-lock argument will prevent npm from creating a package-lock.json file.

Given that we run through scripts/npm.sh anyway, can't we add that argument by default? You could override it with --no-package-lock=false.
If that doesn't work, maybe reset it manually to the previous state after install?

[joaomoreno]

Would that still speed up the builds? I'm afraid this will still be a source of issues since people would have to remember to use --no-package-lock=false when adding a dependency.

[JoshStrobl]

There any movement on this, seeing as nodejs 8 is going to be the new LTS next month?

[felixfbecker]

@joaomoreno

Would that still speed up the builds?

yes, I believe so. The travis.yml could pass in --package-lock to set it to true again.

I'm afraid this will still be a source of issues since people would have to remember to use --no-package-lock=false when adding a dependency.

I don't understand the concern here. As of today, people have to remember to run npm shrinkwrap --dev (and looking at the shrinkwrap.json files it seems like --dev was omitted). Imo passing a command line flag is not worse than running an extra command, wdyt? It also can be set in .npmrc in the repo

[selfus]

Any update on this issue, or still the low priority task on the task list?

[7underlines]

Please do this, so we can get the latest vscode awesomeness on solus. 😎

[theSoenke]

Is there any progress on this issue or idea how this can be solved? This issue exist for a while and Node 8 is now a LTS release.

Passing an additional command line flag is probably still better than no solution at all. Especially when it can be set in .npmrc it should be no problem?

[felixfbecker]

I would like to reiterate that while the behaviour in npm 5 could be better, the same behaviour existed in npm 4 (cross-platform dependencies get removed from shrinkwrap). The only difference in npm 5 is that lockfiles are created automatically. That can be disabled by with package-lock=false in a .npmrc and sticking to manual shrinkwrapping. But scripts/npm.sh install would not take 20min anymore.

[frankdugan3]

This issue is blocking VSCode being updated on Solus Linux. They've been stuck at v1.14.2: https://dev.solus-project.com/T4250

[felixfbecker]

From https://dev.solus-project.com/T4250:

Note: In the event Visual Studio Code is not updated to support NPM5 by the end of November, 2017, it will be deprecated / obsoleted from the repository [...]. It will not return to the repository after such point, as there would be no guarantee that it would continue to work into the future.

[about-code]

@felixfbecker It would help if you put your citation into some context and add a link to its source because without reading the comment before it and the link there, my first guess was you cite some MS employee who is talking about the VSCode GitHub repository. Then it sounds like they'd be planning to withdraw VSCode from GitHub which is not the case, of course. But it was a good shocker for a few seconds 😱

[joaomoreno]

@felixfbecker What kind of issues did you face with yarn?

[felixfbecker]

@joaomoreno I worked quite extensively with yarn when building the TS/JS support for sourcegraph.com a few months ago because at the time npm 5 didn't exist and we needed something fast. Here's a collection of issues I encountered and things I dislike:

• yarn has it's own config, but also reads config from npm's config. When it reads what in what precedence is not obvious. Sometimes yarnrc or npmrc files are not read at all, sometimes env vars are not applied. Yarn's config file says "DO NOT EDIT THIS FILE DIRECTLY" for some reason, but then yarn config sometimes didn't work. npm passes config as env vars to scripts, yarn doesn't.
• Over the course of working with it, there were multiple undocumented sneaky breaking changes. I remember multiple times updating to get a bug fix just to bring in a different fatal bug.
• npm uses a JSON file for the lockfile, which makes it easily machine-readable and editable by tooling. Yarn uses some custom kinda-like-yaml-but-not-really format.
• npm's lockfile saves the structure of node_modules, which means even if your team is on different versions of npm, everyone is guaranteed to get the exact same node_modules structure. Or if you need dependencies to be laid out in certain ways (e.g., to workaround an issue of packages requiring the same package and needing the same instances) that is possible - as long as you can produce the structure and lock it, everyone will get it. Yarn's lockfile however is coupled to the algorithm in the version of yarn.
• yarn was a pain to get into containers, especially Alpine-based, and wants to be installed with the OS package manager. npm on the other hand is simply bundled with Node.
• yarn's documentation was lacking.
• yarn requests does all npm registry requests through a Facebook-hosted proxy.
• yarn does small things different from npm in unexpected ways. For example, npm writes a lot of useful metadata into installed package.jsons (_requestedBy, _from, _integrity, ...), yarn doesn't. npm adds a gitHead field to package.jsons when publishing inside a git repository which can be used to know exactly from what commit was published - yarn doesn't (even though it is returned by the registry it is actively stripped).
• saw countless issues with weird v8 caching, child processes not being killed correctly, writing to files concurrently, ...
• for some reason the yarn decided to rename all commands and flags (install -> add, --save-dev -> --dev, update -> upgrade). Adds friction when rolling out to a team.
• yarn was a pain to get working with private npm packages, which is partly related to the first point.
• for some things people always had to still use npm - back then it was publishing, now it still is creating tokens, ...
• yarn has trouble keeping up with latest npm features, e.g. you can't publish packages if you use 2FA
• yarn's programmatic API is undocumented (it officially doesn't exist) and has no TypeScript typings (as it uses Facebook's Flow). npm has a whole library and TS typings exist.
• there are more recent issues I've heard of from other teams - like not being able to generate code coverage when running scripts through yarn, @types package installation being broken (because of different behaviour to npm), ...

Note that our use case definitely was a stress test for any package manager, things might have improved and not everything may apply to this project. My takeaway was that yarn felt like unstable software and I am more comfortable with the official npm. I see some similarities to node vs iojs - yarn showed off a lot of useful things (default lockfiles, offline mode, great perf) that are now in the core npm.

[joaomoreno]

That's a LONG list, thanks for narrowing it down. Though I can see a few points not applying to us.

Over the past 24h I've been experimenting with Yarn on our repo and the results are pretty good... The cross-platform issue, which is still unresolved in npm5, works perfectly. Speed is great.

Overall it looks pretty great. I have yet to find an issue which I couldn't really get out of.

Here's it, if you want to take a look: https://github.com/joaomoreno/vscode/tree/yarn

[JoshStrobl]

@joaomoreno Would really like to see yarn be utilized for resolving the NPM5 issue, so I can get the Solus package updated without (in my opinion) nasty workarounds like installing NPM4, and without deprecating it from the repo and requesting people use a Snap instead. If it gets to a usable state for you, feel free to ping me and I'l be happy to test.

[felixfbecker]

I definitely prefer yarn over npm 4 any day.

[joaomoreno]

Closing in favour of #38481

[JoshStrobl]

Really thrilled about this getting landed. Thank you @joaomoreno.