Use "*" as a valid "bundleDependencies" value that will bundle all dependencies. #1

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

grncdr commented Mar 31, 2012

When checking dependencies into source control, it's handy to have them in bundleDependencies as well, for commands like npm pack. However, maintaining two identical lists of dependencies is rather un-DRY, so it's nice to just say "bundle ALL the dependencies".

Obviously this is inappropriate for a library module being published to the public registry, so if you're interested in merging this functionality I'd be happy to open another pull request adding a strong warning to the npm documentation about when to use this feature.

Owner

isaacs commented Apr 22, 2012

What about just saying that any time there's a string as bundleDependencies, then it's treated as a pattern to be matched against all deps?

So, then you could do stuff like:

{ "bundleDependencies": "*" }

to bundle them all, or

{ "bundleDependencies": "!{a,b,c}" }

to exclude certain ones.

It'd be fine to add minimatch as a dep to fstream-npm, since fstream-ignore requires it anyway.

grncdr commented Apr 22, 2012

This would fulfill my use-case, but my gut reaction is YAGNI. I admit I'm probably ignorant of the various things npm is used for, but I can't think of many use-cases where "include all my dependencies, except these few" is desirable... maybe for ignoring globally installed binary dependencies? (e.g. database drivers)

Still, if you'd rather it behave as a minimatch pattern I'm happy to update this pull request.

Owner

isaacs commented Apr 23, 2012

Yagni is exactly the point. I do actually need this, for npm itself :)

It has a devDependency on ronn, but I don't want to ship that in the tarball.

grncdr commented Apr 23, 2012

Ah, but the "*" is only bundling "dependencies", not "devDependencies", since these should not be required to run your package. So the patch as it is does what you want, but can't automatically bundle devDependencies (or optionalDependencies).

Adding minimatch and having the pattern match against all dependencies could work, but that requires the user to blacklist devDependencies (and possibly optionalDependencies) they don't want bundled. Not very DRY again...

grncdr commented Apr 23, 2012

As a third option, since npm already uses these 3 categories of packages, maybe it could recognize those category names in the bundleDependencies array. So you could have something like:

{ "bundleDependencies": ["dependencies", "optionalDependencies"] }

or, if "a" and "b" are devDependencies you do want bundled, but you don't want all devDependencies:

{ "bundleDependencies": ["a", "b", "dependencies"] }
Owner

isaacs commented Apr 23, 2012

No, because if you have a dependency named "dependencies", then that'll fail. The logic should be very simple for this:

  1. If it's an array, it's an array of folder names. Bundle all node_modules/X where X is in the array.
  2. If it's a string, it's a pattern to match against the folders in node_modules. Bundle all node_modules/X where X matches the pattern.

There should be no magic about whether something is a dependency or an optionalDependency or any of that. It should just be: "Is it in node_modules? Should it be bundled?" and that's it.

grncdr commented Apr 23, 2012

I understand now, for some reason I thought you were talking about pattern-matching against package names from the dependencies object, not folders in node_modules. I'll write it the way you described and update the PR when I get a few spare minutes.

@grncdr grncdr Allow pattern matching bundleDependencies
If bundleDependencies is a string, use it as a minimatch pattern
against the folders in node_modules/
f114c86

grncdr commented Apr 24, 2012

For reference, there's a corresponding pull request for npm here: isaacs/npm#2395

Contributor

othiym23 commented Sep 9, 2015

I know there's been a lot of back and forth on this PR, which is part of the reason why it's stayed open for so long, but I don't think it makes sense to leave it open any longer. For as convenient as it is, it's still likely to be a confusing user experience, edging towards being a footgun (I'm thinking here of files, which does support globbing of include patterns and also does confuse the heck out of many, if not most, people who encounter it for the first time). I see from npm/npm#2395 that this is the conclusion you eventually came to as well.

Anyway, thanks for putting this together, and thanks for your time and patience in talking it through. 🍄

othiym23 closed this Sep 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment