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

docs: bundledDependencies can be a boolean #4361

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

forty
Copy link
Contributor

@forty forty commented Feb 2, 2022

I discovered when reading the code of npm-bundled [1] that bundledDependencies could be a boolean and that this has the same effect as bundling all dependencies. This seems convenient, as this is probably the main use case for that property, and it already works perfectly :)

Also, as a very meta change, it turn out npm itself seems to bundle all its dependencies, which mean it can take advantage of this to be a bit terser in the package.json, and avoiding the risk of forgetting to add a future new dependency.

[1] https://github.com/npm/npm-bundled/blob/main/index.js#L112

@forty forty requested a review from a team as a code owner February 2, 2022 16:57
package.json Outdated Show resolved Hide resolved
@wraithgar wraithgar changed the base branch from latest to release-next February 2, 2022 17:44
@wraithgar wraithgar changed the title Document the fact that bundledDependencies can be a boolean and use in in npm's own package.json docs: bundledDependencies can be a boolean Feb 2, 2022
@ljharb
Copy link
Contributor

ljharb commented Feb 2, 2022

oof, this seems like a dangerous shortcut, especially since outside of npm itself, nobody should really be using bundledDeps at all. maybe instead of documenting it, we could pursue removing it?

@forty
Copy link
Contributor Author

forty commented Feb 3, 2022

I'm not sure how this can be dangerous, it seems to do exactly what you would expect it to do, isn't it?

There are plenty of use cases were you would want to bundle everything, including all use cases where you don't want to distribute your app via the programming language package manager. This is generally the case for everything that's not a library or a dev tool. In particular, when writing backend code in nodejs, it's common to have to move the app on a server for deployment, where most likely there is no internet access, and no package manager installed on the production machines. Some people are using docker for this kind packaging, and even for this use case, having a single dependency-free tgz package that you can ADD in your dockerfile can be super convenient on some CI setup.

I know that many people like using front end bundlers (esbuild, webpack) for this kind of thing that bundles everything in a single file, but bundledDepencies seem much simpler and works well.

I'll update the PR to rebase and remove the non doc commit.

This has been possible since this commit in `npm-bundled`:
npm/npm-bundled@101a94d
@forty forty force-pushed the forty/bool-bundle-dependencies branch from d860f1d to eb5bc3e Compare February 3, 2022 08:27
Copy link
Member

@wraithgar wraithgar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This documents the current reality, changing it is outside the scope of this PR.

@wraithgar wraithgar merged commit 0b0a7cc into npm:release-next Feb 3, 2022
@ljharb
Copy link
Contributor

ljharb commented Feb 3, 2022

@forty frontend bundlers aren't impacted by bundledDeps either way, because they have to build a single javascript file suitable for consumption in browsers. For Docker, you can just tar up the existing app with node_modules; bundledDeps only really matters if you're using npm publish.

@wraithgar while that is true, "documenting the current reality" is not always a good thing. Some functionality is best left undocumented.

@forty
Copy link
Contributor Author

forty commented Feb 4, 2022

@ljharb you are not wrong in the sense that today I can simply npm ci --prod then tar my js file with my node_module folder to achieve something similar to npm pack with "bundledDependencies": true (and it is what I do indeed)

However, and the main reason why I ended up reading the code in the first place, I would like to use the workspace feature of npm. And with the workspaces, bundling a tarball with only the dependencies required for a given package of the monorepo is much trickier. And I hoped that bundledDependencies would to that for me.

Now it turns out that bundledDependencies does not work well with workspaces at the moment :) (see #2207 and #3466). But when it does, I do feel the feature would be useful.

@forty
Copy link
Contributor Author

forty commented Feb 4, 2022

(And to add to this, I would absolutely agree if you would argue that adding a --bundle-deps parameter to npm pack would be nicer than a property in the package.json)

@tomasgvivo
Copy link

@ljharb you are not wrong in the sense that today I can simply npm ci --prod then tar my js file with my node_module folder to achieve something similar to npm pack with "bundledDependencies": true (and it is what I do indeed)

However, and the main reason why I ended up reading the code in the first place, I would like to use the workspace feature of npm. And with the workspaces, bundling a tarball with only the dependencies required for a given package of the monorepo is much trickier. And I hoped that bundledDependencies would to that for me.

Now it turns out that bundledDependencies does not work well with workspaces at the moment :) (see #2207 and #3466). But when it does, I do feel the feature would be useful.

Bump this... I'm having the same problem when trying to make docker images for services inside a monorepo. Packing workspaces with bundled dependencies would help a lot with that!

(And to add to this, I would absolutely agree if you would argue that adding a --bundle-deps parameter to npm pack would be nicer than a property in the package.json)

Also, this would be a nice addition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants