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

[FR]: npm_package(bundle_dependencies = ["my_dep"]) #891

Closed
1 task
dgp1130 opened this issue Feb 20, 2023 · 4 comments
Closed
1 task

[FR]: npm_package(bundle_dependencies = ["my_dep"]) #891

dgp1130 opened this issue Feb 20, 2023 · 4 comments
Labels
discussion needed Discussion required to progress further enhancement New feature or request

Comments

@dgp1130
Copy link
Contributor

dgp1130 commented Feb 20, 2023

What is the current behavior?

npm_package() builds only the current package and drops all of its dependencies, assuming they will be linked separately. This is usually the desired behavior, but can be limiting when trying to share utilities within a monorepository. Consider:

packages/
    lib/
        index.js
        package.json
    utilities/
        index.js
        package.json

If a file in lib depends on utilities, then it either needs to either:

  1. Make a relative import which looks like ../../../../utilites/....
  2. Leverage link_workspace_root (which I believe is generally discouraged).
  3. Depend on //:node_modules/utilities from npm_link_package().

The last approach works great locally, but requires the utilities package to be published if the lib package is published. Users then need to install both packages and link them together at runtime, even though utilities may not be public API or be trivial enough to not justify its own published package.

Describe the feature

I think the solution here is to leverage NPM's bundleDependencies feature. We can define an npm_package() containing a list of bundle_dependencies which are included in the output.

npm_package(
    name = "lib",
    srcs = [...],
    bundle_dependencies = ["//:node_modules/utilities"],
)

This would generate a directory structure like:

lib/
    index.js
    package.json
    node_modules/
        utilities/
            index.js
            package.json

It would also update the package.json to include bundleDependencies: ['utilities'], which would tell NPM to keep the bundled utilities package.

I tried doing this in user-space and kinda got it to work. I was able to generate the node_modules/ tree, though it gets dropped by npm_package() which sets exclude_srcs_patterns = ["**/node_modules/**"] by default. After clearing that attribute, the package contained the bundled dependency and I was able to use it via npm_link_package() from a separate workspace.

I didn't try publishing the bundled package, but cd-ing into the built artifact and running npm pack for some reason drops the node_modules/ directory, yet if I copy the directory out of Bazel's output tree, and then run npm pack, it does contain node_modules/. I'm not sure if there's a file system issue here or something else going wrong.

I'm also not entirely sure how transitive dependencies should work. Testing this out independent of Bazel, npm install of a bundled transitive dependency seems to lay out the directories like:

foo/
    node_modules/
        bar/
            node_modules/
                baz/

Though it also seems to support:

foo/
    node_modules/
        bar/
        baz/

I suspect the first option would be desirable, since two packages could bundle different "versions" of the same dependency, and they should each get their bundled implementation.

I don't think my implementation is doing this feature correctly, since it lays out a flat node_modules/ structure. I'm also not entirely sure how the dependency structure of linked packages works in @aspect_rules_js. I also feel like the deps attribute of npm_link_package() might be relevant here, but I couldn't find any documentation about it and don't understand how it is supposed to work.

While I think bundled dependencies would be really cool, my ultimate goal is around sharing code in my monorepository. Currently I'm doing everything through relative imports, which doesn't scale great. I was under the impression the recommendation in an @aspect_rules_js world was to make private packages and manually link them. That works fine locally, but I don't want to publish all these internal, utility packages and would rather inline them in the public packages where needed. That's what led me to find a way to bundle dependencies in an NPM package, I'm not sure if there's a better approach for sharing code.

Fund our work

@devversion
Copy link
Contributor

My stance in such scenarios would be that you end up manually bundling these shared private libraries- if you don't want to publish them to npm. That is something you could do manually via e.g. esbuild before exposing the npm package.

If other parts of the monorepo then would rely on the NPM artifact, that would incur the cost of running the bundler. Previously, with rules_nodejs we were solving this by having a clear separation of NPM artifacts and dev-linked packages. In either case, we would use a module name, like @my-workspace/private/utils, but then for NPM it may be bundled, but for dev-mode it may point to the basic-transpiled TS output.

@gregmagolan gregmagolan added discussion needed Discussion required to progress further and removed untriaged Requires traige labels Mar 10, 2023
@gregmagolan gregmagolan moved this to 📋 Backlog in Open Source Mar 10, 2023
@gregmagolan
Copy link
Member

gregmagolan commented Mar 11, 2023

Structurally the npm_package rule is designed to be an un-opinionated packaging rule (with some exceptions for things like filter out node_modules folder that users might accidentally have in their deps graph but shouldn't be included in the contents in the default case). The implementation is just a simple wrapper around the copy_to_directory rule & API from our bazel-lib.

copy_to_directory_bin_action(

It doesn't currently generate or modify the package.json file and generally leaves it up to the user to configure the contents of the package.

It sounds like you've got something working in user space. Could this be generalized into a rule that proceeds npm_package that could then be passed to the npm_package sources to get the updated package.json and any files needed to go along with it for the bundled dependencies?

@dgp1130
Copy link
Contributor Author

dgp1130 commented Mar 11, 2023

This is probably do-able in user-space if we'd rather say it's out of scope for @aspect_rules_js. My solution kind of works, but I don't think I understand enough of how npm_package() and link_npm_package() to do it in a fully reliable way. As I mentioned originally, I'm pretty sure I'm not handling transitive dependencies correctly but I'm not sure what the right way of working with @aspect_rules_js data structures is here.

However, I do think the general use case of "Sharing code within a monorepo without having to relative import really far away or publish every utility package" is something that @aspect_rules_js should own in some capacity, though it's quite possible this is not the right solution to that problem.

@devversion and I have been chatting about this a bit. I do like his point that we could run a bundler to preprocess the package, I could see that as a viable solution (and probably makes sense for Angular), but it does have some undesirable side effects. Bundlers typically don't have a mode for "vendor this one dependency in my package", so they are likely doing other work which needs to be turned off or worked around. For example, I don't think this use case benefits from bundling all the JavaScript into a single file, it's also not a strict file-by-file transform. You'll likely have to live with some undesired changes to the package output you didn't want in the first place. Basically, I'm not sure how you would get ESBuild to do the right thing here without biting off a bunch of complexity or annoying trade-offs you shouldn't need to make. I was hoping bundleDependencies might be a simpler solution to this problem than running through a bundler.

The point about dev / prod builds is relevant here though. @aspect_rules_js doesn't have any built-in concept of dev / prod builds and the design of link_npm_package() makes it difficult to have two different versions of the same package. In @rules_prerender I haven't had the need for splitting a package's build like that, so bundleDependencies is a nice approach in that model (I don't need a dev/prod build), but certainly I can see it being insufficient for a use case like Angular.

One fundamental problem with bundling dependencies (whether via bundleDependencies or a separate bundle step) is that they duplicate the bundled dependencies across every package which bundles them. This can work fine, but if data passes between the two instances of the dependencies it can lead to strange behavior given that you'll have two instances of the dependencies which are not necessarily interoperable. This is ultimately the trade off being made for bundling a dependency in an NPM package, but it is a foot-gun for users for sure.

This is a little rambling and not really a cohesive comment, but hopefully that makes some sense. My goal here was to propose bundleDependencies as one possible mechanism for sharing internal packages without having to publish them. It's possible I'm jumping too far into one possible solution, when maybe this issue would be better phrased as a more general "How do I share code in a monorepo?" given the general requirements of:

  1. Relative importing specific files which are "far away" is awkward.
  2. Absolute imports aren't really possible / are discouraged since link_workspace_root isn't a thing anymore (also would probably complicate the npm_package() step).
  3. Internal code shouldn't need to be published as distinct NPM packages.

I remember some Slack thread where Greg or Alex basically said that the intended solution to this to create internal NPM packages. That can work locally, but the only way I see that working when published is by publishing those internal packages as well which I'm hesitant to do for internal utilities. Maybe the solution is bundleDependencies, maybe it's running a separate bundler tool, or maybe something else I'm not seeing?

@alexeagle
Copy link
Member

I've used ncc for this, and even published the code https://github.com/aspect-build/bazel-examples/blob/pkg/vercel_pkg/defs/vercel_pkg.bzl and a blog post so I think this can be considered resolved.

@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Open Source Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion needed Discussion required to progress further enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

4 participants