postinstall script depends on devDependencies

[zkochan]

The postinstall script executes ./dist/postinstall.js which depends on packages specified as devDependencies. Those packages should be either regular or peer dependencies as they won't be installed with vscode-riprep.

Related: pnpm/pnpm#833

[roblourens]

Yeah, it's a bad hack because those dependencies are only needed for install, and we don't want to ship them with vscode. Need to find a better way to do that...

[roblourens]

This actually isn't fixed yet, I rolled those changes back.

@joaomoreno, wondering if you have any ideas here - the method I currently use to distribute ripgrep in vscode is not great. We have vscode-ripgrep as a dependency, and during its postinstall, it downloads the rg binary. It has some dependencies that it uses to download ripgrep, but they aren't needed at runtime, so they are marked as devDependencies, to keep vscode from shipping them. So when vscode-ripgrep is installed in vscode, of course these dependencies aren't actually enforced, but it works because they're required by gulp-atom-electron and other modules in vscode. Today it broke because we updated yauzl to an incompatible version and I clearly need to clean this up.

Options

• Use vscode-ripgrep more like gulp-atom-electron. Call it from vscode's gulpfile and download ripgrep to .build/ or somewhere. Then I'm not sure where it should live in the actual package.
  • Or get rid of vscode-ripgrep as a separate module and move this code into vscode's gulpfiles
• Change vscode-ripgrep's dependencies to 'dependencies', but filter them out using cleanNodeModule. But it brings in a lot of dependencies, which may be required at runtime by something else.

[joaomoreno]

Why not implementing the whole thing in pure Node? You don't really need all those dependencies. You could use the https module to download the exact version you are looking for. And you could simply upload Gzipped archives of rg, which you can use the zlib module to extract.

[joaomoreno]

cc @sandy081

OK, long story:

This morning, the extension viewlet was completely broken: it didn't render extensions. @sandy081 looked into it and found it the shared process failed to load altogether:

That yauzl reference at the top quickly caught my eye. I looked around and found microsoft/vscode#30759 which was merged in yesterday. I promptly reverted it with microsoft/vscode#30923. That should fix it, I thought.

... but it didn't.

All builds started failing after that:

2017-07-18T10:51:04.0713580Z Downloading to /Users/code/tfs/agent3/_work/_temp/vscode-ripgrep-cache/ripgrep-0.5.1-patch.0-darwin-x64.zip
2017-07-18T10:51:04.0736430Z Downloading ripgrep-0.5.1-patch.0-darwin-x64.zip...
2017-07-18T10:51:06.3594460Z Unzipping to /Users/code/tfs/agent3/_work/2/s/node_modules/vscode-ripgrep/bin
2017-07-18T10:51:06.3619780Z /Users/code/tfs/agent3/_work/2/s/node_modules/vscode-ripgrep/dist/download.js:71
2017-07-18T10:51:06.3641060Z             zipFile.readEntry();
2017-07-18T10:51:06.3653060Z                     ^
2017-07-18T10:51:06.3659090Z 
2017-07-18T10:51:06.3670720Z TypeError: zipFile.readEntry is not a function
2017-07-18T10:51:06.3690660Z     at yauzl.open (/Users/code/tfs/agent3/_work/2/s/node_modules/vscode-ripgrep/dist/download.js:71:21)
2017-07-18T10:51:06.3711100Z     at /Users/code/tfs/agent3/_work/2/s/node_modules/yauzl/index.js:31:7
2017-07-18T10:51:06.3722930Z     at /Users/code/tfs/agent3/_work/2/s/node_modules/yauzl/index.js:96:14
2017-07-18T10:51:06.3734700Z     at /Users/code/tfs/agent3/_work/2/s/node_modules/yauzl/index.js:342:5
2017-07-18T10:51:06.3753000Z     at /Users/code/tfs/agent3/_work/2/s/node_modules/fd-slicer/index.js:32:7
2017-07-18T10:51:06.3772520Z     at FSReqWrap.wrapper [as oncomplete] (fs.js:629:17)

Oh, vscode-ripgrep now uses new API from yauzl. Easy, I reverted that in my fork: joaomoreno@64a2f05 and updated our package.json and npm-shrinkwrap.json to use that version: microsoft/vscode@b272deb

Fixed! I've triggered new insider builds.

Questions:

• Why can't the shared process find buffer-crc32, which yauzl depends on? Do we remove it during our build and, if so, why?
• It seems that yauzl violated the semver rules by breaking API without incrementing the major version. Why does vscode-ripgrep not lock its dependency to yauzl? If that would happen, I believe npm would place a local copy of yauzl inside vscode-ripgrep's node_modules folder, stuck on any version we'd like.

Next steps:

• Either make vscode-ripgrep not depend on yauzl or lock its dev dependency to yauzl, preventing a clash with vscode's version. I keep my suggestion: drop all dependencies and write this downloading code in pure node lib.
• Revert my commit microsoft/vscode@b272deb by updating vscode-ripgrep to that new version.

[joaomoreno]

Why can't the shared process find buffer-crc32, which yauzl depends on? Do we remove it during our build and, if so, why?

After digging a bit more, this happens because npm-shrinkwrap.json in microsoft/vscode#30759 misses the addition of the new dependencies of yauzl, including buffer-crc32.

Will investigate more!

[roblourens]

Sorry you had to deal with that! You probably could have just pulled in [email protected] instead of forking it. But yes yauzl violates semver 😝.

Why does vscode-ripgrep not lock its dependency to yauzl?

vscode-ripgrep has these all of its dependencies marked as devDependencies, so they are all ignored when we install it for vscode. If something else wasn't depending on yauzl, it wouldn't have yauzl at all.

Rewriting it without any dependencies is a good idea, but I'd rather use the power of npm if I can rather than reinventing the wheel. How about running vscode-ripgrep from our gulpfile and copying ripgrep into another folder somewhere?

[joaomoreno]

You probably could have just pulled in [email protected] instead of forking it.

Doh!

I also would like to keep using the power of npm. I meant it as "keep the postinstall, just dont depend on node modules to implement it". Why would dropping the dependency to yauzl go against that? I thought you create the zip files yourself and upload them to GitHub.

In any case, feel free to (properly) upgrade yauzl to the latest version in our core, adopting the right API changes and updating the shrinkwrap file accordingly. If you do this, we can move back to 0.0.15.

[roblourens]

"keep the postinstall, just dont depend on node modules to implement it"

I understand, but I mean the power of the npm ecosystem. Besides yauzl, I'd rather keep using github-releases and the other deps to make life easier, rather than reimplement them. That just seems like an extreme solution.

[joaomoreno]

Then two options:

• Adopt the latest yauzl in our core.
• Or, report the semver breakage to yauzl, in order to avoid npm assuming that the two versions are indeed compatible.

[roblourens]

The semver breakage is annoying but didn't technically contribute to this issue, because the vscode-ripgrep deps are not respected by npm at all. In any case I'll work on taking the latest yauzl.

[joaomoreno]

Actually the deps are respected by npm. Core is locked into yauzl 2.3.1. vscode-ripgrep now depends on 2.8.0. Those two should be API compatible right? So, npm just keeps that 2.3.1 version around for vscode-ripgrep to use. That's how it fails, and that's why I had to revert.

[roblourens]

Actually vscode-ripgrep's deps are totally ignored by npm, because they are devDependencies, not dependencies as they should be. If something else doesn't require them then they won't even be installed. So we can upgrade yauzl but this won't help when we upgrade it again to 3.0 or something.

So I'm saying the best solution is to make vscode-ripgrep a devDependency from vscode, which lets us make vscode-ripgrep's deps dependencies. And it should download ripgrep to somewhere like .build/ making it work more like gulp-atom-electron. I'm just not sure where it should go in the final vscode package or how that should work.

[joaomoreno]

But we do install our dependencies' devDependencies. We never run npm with --production. Otherwise many other things will break. 😕

[zkochan]

devDependencies are only installed for the project. devDependencies of dependencies are never installed.

[joaomoreno]

Oh wow, it's the first time I ever realise that. You're right.

@roblourens Did you take a look at https://www.npmjs.com/package/postinstall-build yet?

[roblourens]

Whoa, that's interesting, I will try it out.

This would be an issue though:

The prune command is broken in npm 4.1.x–4.5.x, and is unable to correctly prune devDependencies. Thus, when postinstall-build is finishing up, it leaves behind extraneous packages.

[zkochan]

I might not have the whole picture here but at the beginning of the thread it was mentioned that

it works because they're required by gulp-atom-electron and other modules in vscode

if those deps can be resolved from higher in the dependency tree, when vscode-rigrep is installed, then they can be peerDependencies

[joaomoreno]

True, yet not all of those would make sense to be dependencies of vscode. :/

[roblourens]

What's the status of switching to npm 5 officially? I see remaining issues around it. It sounds like postinstall-build would be the perfect solution if npm prune removes the devDependencies of dependencies in npm 5.

[joaomoreno]

We are blocked on npm5: microsoft/vscode#30134. Usually we tend to wait months before adopting a major npm version. I already spent way too long on that this month. 😞

[roblourens]

Still thinking about this. peerDependencies solution is the fastest but leaves room for issues in the future. Other crazy ideas:

• "inline" the node_modules, ie publish them as part of the vscode-ripgrep npm package, then have vscode clean that dir with cleanNodeModule
• Forget about downloading the binary - publish all binaries as part of the npm package, and remove the binaries for the other platforms with cleanNodeModule

Also still thinking about reimplementing it without any npm dependencies, as suggested above. Annoying thing is that if github changes something I can't just rev github-releases to fix it, I'll have to debug it myself.

[roblourens]

vscode-ripgrep's npm package now includes _node_modules/, renames it to node_modules/ before postinstall, then deletes its node_modules/.

Seems to be working fine, I'll verify some more, and merge it, and update yauzl in vscode tomorrow.

[zkochan]

If you move your node_modules to dist/node_modules before publish, it will be published with the package and will work without renaming (npm ignores node_modules only in the root). That is how we bundle deps for pnpm. bower also does it this way. There is a package that does this: publish-packed

NOTE: this solution works only if the dependencies have no install scripts that should be executed after installation

[roblourens]

Cool, I wish I'd seen that package earlier yesterday! I'm opening a new issue to switch to publish-packed and closing this one.