microsoft/vscode-vsce

Ignore dependencies of devDependencies

SrTobi opened this issue · 20 comments

Are dependencies of devDependencies ignored?

When I use npm install and then vsce ls a lot of node_modules are still listed. the devDependencies are ignored but their dependencies are not.

Wouldn't it be better to include all production dependencies and their production dependencies and ignore all other modules?

Very annoying! The docs state:

Note: Development dependencies listed in devDependencies will be automatically ignored, you don't need to add them to the .vscodeignore file.

But when I check the installed extension:

~\.vscode\extensions\felixfbecker.php-debug-1.1.0$ npm ls
php-debug@1.1.0 C:\Users\felix\.vscode\extensions\felixfbecker.php-debug-1.1.0
├── commander@2.3.0 extraneous
├── debug@2.2.0 extraneous
├── diff@1.4.0 extraneous
├── escape-string-regexp@1.0.2 extraneous
├── file-url@1.0.1 extraneous
├── glob@3.2.3 extraneous
├── growl@1.8.1 extraneous
├── iconv-lite@0.4.13
├── jade@0.26.3 extraneous
├── mkdirp@0.5.0 extraneous
├── moment@2.11.1
├── supports-color@1.2.0 extraneous
├── url-relative@1.0.0
├── vscode-debugadapter@1.2.0
├── vscode-debugprotocol@1.2.1
└── xmldom@0.1.20

See all the "extraneous" packages installed, those are dev-dependencies-dependencies.

Here is a workaround, put this is package.json scripts:

"vscode:prepublish": "npm install && npm run compile && npm prune --production"

Before publishing, it will install everything, then compile, and at the end remove all devDependencies. Unfortunately, there is no postpublish hook, so you will have to run npm install after publishing again.

Great idea 👍

That is really cool, @felixfbecker, didn't know about npm prune.

Not really a fix, though. Perhaps something like this (opened before I realized there was a separate repo for vsce): microsoft/vscode#2114

I dont think reinstalling on publishing is the right solution.
All vsce needs to do is add the output of npm list --only=dev --parseable --depth to the list of ignored folders.

@felixfbecker even better, yes. I was not aware of that option. I'm assuming that eliminates any cross-dependencies (i.e. same package indirectly required by both dependencies and devDependencies). Certainly saves time for bigger extensions from having to copy files and re-install potentially dozens (or hundreds) of dependencies. Cool.

I'm assuming that eliminates any cross-dependencies (i.e. same package indirectly required by both dependencies and devDependencies)

Good point. So then the other way around, include only from node_modules what's in
npm list --production --parseable --depth

npm list --only=dev --parseable --depth

This is pretty epic, didn't know about this either. Would you like to submit a PR? 👍

While it doesn't solve the problem in the save way, a cursory glance seems like it would also work (an existing PR): #58

Hmm why not combine both solutions? In fact the code in my pr also uses npm list, but without the --only=dev --parseable --depth. We could put those args into the npm.list function (called here) and still have a nice return value. The hole determineProductiveModules function could than be removed

@SrTobi That was my idea :) Of course it makes sense to use the NPM programmatic API instead of the CLI. But we shouldn't reimplement what NPM already provides.

It is just important that we parse the production packages and only include them, instead of parsing the dev packages and exclude those. Otherwise you might have packages ignored that were both in your dev-dependencytree and your production-dependencytree (think about it, almost everyone depends on lodash somewhere in their dependency chain).

Hmmm in fact I am actually doing this. I take all productive dependencies and extension files and then do the normal exclusion process (of course without the devDependencies).

The idea to let npm output only productive dependencies instead of finding them by ourselves is pretty nice though.

Fixed by #58

v4run commented

Hi, I am still facing this issue for an extension I wrote. vsce package makes a 5MB file. The only thing that worked for me is

"vscode:prepublish": "npm install && npm run compile && npm prune --production"

which creates a much smaller file.

I am using vsce@1.1.0 and npm@3.7.1

I did some debugging and it is definitely a npm problem. npm simply ignores the --production argument and prints every dependency regardless whether it is a productive dependency or a dev dependency. In npm@3.6.0 it worked well.

I also checked the npm command we are currently using (in https://github.com/Microsoft/vscode-vsce/blob/master/src/npm.ts#L4) and I think the --depth should be removed.

npm list --production --parseable --depth
  1. --depth is not correct as it needs a value like --depth=0.
  2. The original idea was to use --depth=0 (I had that in my PR), but that would be terrible. It is actually not, because npm simply ignores the --depth argument when --parseable is also used, but that's a bug, I think. The effect, if it would work, is that dependencies of direct dependencies are also omitted. Especially productive dependencies of productive dependencies. That's of course not what we want.

I opened an issue for the --productive problem: npm/npm#11530
And one issue for the --depth=0 + --parseable: npm/npm#11531

@SrTobi You need to specify depth because depth could be set through npm user config (I always set it to 0 to avoid the cluttered trees). But of course it should be something like 99999, not 0.

ahh ok, didn't know about that. Then we should indeed set it to 9999.