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

[Bug]: js_image_layer puts both first-party and third-party code into same layer #1641

Closed
Tracked by #1671
matthewjh opened this issue Apr 11, 2024 · 3 comments
Closed
Tracked by #1671
Labels
enhancement New feature or request

Comments

@matthewjh
Copy link

What happened?

Hi. js_image_layer has a separation of "app" and "node_modules" layers. The problem is that when 1st-party libraries are used as linked node_modules via the npm_link rules or pnpm workspaces - probably very common for rules_js users - these 1st party libraries are included amongst all the 3rd party node_modules in the node_modules layer, meaning you have 1 layer containing most of the code (100-200mb) and that whole layer changing whenever one of the 1st party libraries changes even slightly.

See https://github.com/aspect-build/rules_js/blob/main/js/private/js_image_layer.bzl#L241

I ended up patching this to check for our "scope" prefix, so our first-party packages would go into the app layer instead of node_modules. However, under this issue it probably makes sense to revisit this bifurcation. Given that rules_js can distinguish between packages that originated in the workspace and externally (e.g. from npm), it would be perhaps nice to see a separate layer for everything "external" and "internal", which appears to better capture the intent of the "app" and "node_modules" layers to separate "infrequently changing" from "frequently changing".

Version

Development (host) and target OS/architectures:

Output of bazel --version:
7.1.1
Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file:

Language(s) and/or frameworks involved:

http_archive(
    name = "aspect_rules_js",
    sha256 = "d7500d59712accca9622bce723b8ea596b92fb9b4ff4a4dbd77ed353b9d29f34",
    strip_prefix = "rules_js-1.40.1",
    url = "https://github.com/aspect-build/rules_js/releases/download/v1.40.1/rules_js-v1.40.1.tar.gz",
    patches = [
        "//tools/bazel-patches:aspect_rules_js-image-layer.patch",
    ],
    patch_args = ["-p1"],
)

How to reproduce

No response

Any other information?

No response

@matthewjh matthewjh added the bug Something isn't working label Apr 11, 2024
@github-actions github-actions bot added the untriaged Requires traige label Apr 11, 2024
@gregmagolan
Copy link
Member

gregmagolan commented Apr 11, 2024

dTricky one. From the rule's perspective when it looks in the JsInfo provider all node_modules is just a depset of Files under a common path. Being able to discern between the two would likely require some provider changes... tho as it happens we're currently working on the 2.x branch where we are free to make breaking provider changes.

One way to go would be to make the provider changes so that js_image_layer can automatically separate out 1p and 3p npm deps. The other possibility is to add the functionality to js_image_layer to create multiple node_modules layers based on a regex or glob of the package name. You could argue the 2nd way is more better solution as it generic and would allow users to also split their 3p deps into multiple layers if needed.

@gregmagolan gregmagolan added enhancement New feature or request and removed bug Something isn't working untriaged Requires traige labels Apr 11, 2024
@gregmagolan gregmagolan mentioned this issue May 6, 2024
21 tasks
@gregmagolan
Copy link
Member

gregmagolan commented May 7, 2024

Looks like this will be relatively easy to land on the 2.x branch and released with rules_js 2.0. It goes along with js_image_layer support for direct linked 1p deps via js_library that was added in #1646. Release tracking issue is here: #1671

@gregmagolan
Copy link
Member

Fixed with #1712. Fix will be included in the rules_js 2 release

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

No branches or pull requests

2 participants