Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ignore dependencies of devDependencies #52

Closed
SrTobi opened this issue Dec 15, 2015 · 20 comments
Closed

Ignore dependencies of devDependencies #52

SrTobi opened this issue Dec 15, 2015 · 20 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug
Milestone

Comments

@SrTobi
Copy link
Contributor

SrTobi commented Dec 15, 2015

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?

@felixfbecker
Copy link

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.

@felixfbecker
Copy link

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.

@SrTobi
Copy link
Contributor Author

SrTobi commented Jan 17, 2016

Great idea 👍

@joaomoreno
Copy link
Member

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

@joaomoreno joaomoreno added the bug Issue identified by VS Code Team member as probable bug label Jan 18, 2016
@joaomoreno joaomoreno self-assigned this Jan 18, 2016
@heaths
Copy link
Member

heaths commented Jan 19, 2016

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

@felixfbecker
Copy link

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.

@heaths
Copy link
Member

heaths commented Jan 19, 2016

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

@felixfbecker
Copy link

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

@joaomoreno
Copy link
Member

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

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

@heaths
Copy link
Member

heaths commented Jan 19, 2016

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

@SrTobi
Copy link
Contributor Author

SrTobi commented Jan 20, 2016

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

@felixfbecker
Copy link

@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).

@SrTobi
Copy link
Contributor Author

SrTobi commented Jan 20, 2016

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.

@joaomoreno
Copy link
Member

Fixed by #58

@v4run
Copy link

v4run commented Feb 13, 2016

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

@SrTobi
Copy link
Contributor Author

SrTobi commented Feb 13, 2016

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.

@SrTobi
Copy link
Contributor Author

SrTobi commented Feb 13, 2016

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.

@SrTobi
Copy link
Contributor Author

SrTobi commented Feb 13, 2016

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

@felixfbecker
Copy link

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

@SrTobi
Copy link
Contributor Author

SrTobi commented Feb 13, 2016

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

vazexqi referenced this issue in forcedotcom/salesforcedx-vscode Jul 24, 2017
* Simplify command invocations are npm scripts
* Add shortcuts for running these commands from VS Code
* Use shx to run scripts in platform-agnostic manner
* Add scripts to help with packaging and publishing of .vsix
* Add instructions on how to publish
* Clarify how to run on Windows

@W-4162004@
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

5 participants