feat: add `prod` and `dev` attributes to `npm_link_targets` #2051

jfirebaugh · 2024-12-20T19:53:04Z

Fixes #1879

Changes are visible to end-users: yes

Searched for relevant documentation and updated as needed: TODO
Breaking change (forces users to change their own code or config): no
Suggested release notes appear below: no

Test plan

(TODO) New test cases added

aspect-workflows · 2024-12-20T19:53:30Z

Test

All tests were cache hits

225 tests (100.0%) were fully cached saving 34s.

Test

e2e/bzlmod

All tests were cache hits

5 tests (100.0%) were fully cached saving 539ms.

Test

e2e/gyp_no_install_script

All tests were cache hits

2 tests (100.0%) were fully cached saving 184ms.

Test

e2e/js_image_oci

All tests were cache hits

1 test (100.0%) was fully cached saving 6s.

Test

e2e/npm_link_package

All tests were cache hits

3 tests (100.0%) were fully cached saving 533ms.

Test

e2e/npm_link_package-esm

All tests were cache hits

3 tests (100.0%) were fully cached saving 630ms.

Test

e2e/npm_translate_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Test

e2e/npm_translate_lock_empty

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Test

e2e/npm_translate_lock_multi

All tests were cache hits

2 tests (100.0%) were fully cached saving 268ms.

Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Test

e2e/npm_translate_lock_replace_packages

⚠️ Buildkite build #8855 failed.

Failed tests (1)

//:write_npm_translate_lock_bzlmod_test [k8-fastbuild]🔗

💡 To reproduce the test failures, run

bazel test //:write_npm_translate_lock_bzlmod_test

Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

1 test (100.0%) was fully cached saving 73ms.

Test

e2e/npm_translate_package_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Test

e2e/npm_translate_yarn_lock

All tests were cache hits

1 test (100.0%) was fully cached saving 31ms.

Test

e2e/package_json_module

All tests were cache hits

1 test (100.0%) was fully cached saving 324ms.

Test

e2e/pnpm_lockfiles

⚠️ Buildkite build #8855 failed.

Failed tests (5)

//v101:repos_0_test [k8-fastbuild]🔗
//v54:repos_0_test [k8-fastbuild]🔗
//v60:repos_0_test [k8-fastbuild]🔗
//v61:repos_0_test [k8-fastbuild]🔗
//v90:repos_0_test [k8-fastbuild]🔗

💡 To reproduce the test failures, run

bazel test //v90:repos_0_test //v61:repos_0_test //v60:repos_0_test //v101:repos_0_test //v54:repos_0_test

Test

e2e/pnpm_workspace

⚠️ Buildkite build #8855 failed.

Failed tests (1)

//:repos_test [k8-fastbuild]🔗

💡 To reproduce the test failures, run

bazel test //:repos_test

Test

e2e/pnpm_workspace_rerooted

⚠️ Buildkite build #8855 failed.

Failed tests (1)

//:repos_test [k8-fastbuild]🔗

💡 To reproduce the test failures, run

bazel test //:repos_test

Test

e2e/repo_mapping

All tests were cache hits

3 tests (100.0%) were fully cached saving 410ms.

Test

e2e/rules_foo

All tests were cache hits

2 tests (100.0%) were fully cached saving 137ms.

Test

e2e/runfiles

All tests were cache hits

1 test (100.0%) was fully cached saving 107ms.

Test

e2e/vendored_node

All tests were cache hits

1 test (100.0%) was fully cached saving 114ms.

Buildifier

Buildifier managed files require formatting

--- ./npm/private/npm_translate_lock_helpers.bzl	2025-07-14 23:27:22.975203945 +0000
+++ /tmp/buildifier-tmp-2127870111	2025-07-14 23:32:05.703665597 +0000
@@ -269,7 +269,7 @@
         linked_packages = {}
         importer_links[import_path] = {
             "link_package": _link_package(root_package, import_path),
-            "packages": linked_packages
+            "packages": linked_packages,
         }
         for deps_type in ["deps", "dev_deps"]:
             deps = importer.get(deps_type)
@@ -287,7 +287,7 @@
                     maybe_package = dep_version
                 if maybe_package not in linked_packages:
                     linked_packages[maybe_package] = []
-                linked_packages[maybe_package].append({ "pkg": dep_package, "dev": deps_type == "dev_deps" })
+                linked_packages[maybe_package].append({"pkg": dep_package, "dev": deps_type == "dev_deps"})
 
     patches_used = []
     result = {}
@@ -414,7 +414,7 @@
             if public_hoist_package not in link_packages:
                 link_packages[public_hoist_package] = []
             if name not in link_packages[public_hoist_package]:
-                link_packages[public_hoist_package].append({ "pkg": name, "dev": False })
+                link_packages[public_hoist_package].append({"pkg": name, "dev": False})
 
         run_lifecycle_hooks = all_lifecycle_hooks and (name in only_built_dependencies if only_built_dependencies != None else requires_build)
         if run_lifecycle_hooks:

💡 Run the following to apply the suggested formatting fixes

bazel run //:buildifier

Format

Formatting check has failed

💡 Some formatting failures can be fixed automatically by running the command below, while others may require manual fixes

bazel run //:format -- npm/private/npm_translate_lock_helpers.bzl

ℹ️ A patch file containing the changes has been archived as an artifact of this build

jbedard · 2025-01-03T20:37:44Z

npm/private/npm_translate_lock_generate.bzl

@@ -219,7 +222,7 @@ def npm_link_targets(name = "node_modules", package = None):
    npm_link_all_packages_bzl = [
        """\
 # buildifier: disable=function-docstring
-def npm_link_all_packages(name = "node_modules", imported_links = []):
+def npm_link_all_packages(name = "node_modules", imported_links = [], prod = False, dev = False):


Any other ideas for this API?

It might be nice if we didn't have this scenario where 1/4 of the states is basically invalid and has an odd non-intuitive default. Maybe just throw when false/false and then the other 3 states are valid and understandable?

dev = True|False|None or something like that is one alternative, although the word "dev" in the API without "prod" in the API is confusing imo. Or type = "dev"|"prod"|None but I'd want a term better then "type".

Actually is this API simply a copy of npm_translate_lock? In which case we should do exactly as you have and my suggestion can be considered in rules_js v3 or something like that...

Yeah, it's just following the prod/dev API from npm_translate_lock. I agree that an API that excludes invalid states by design would be better for v3.

This current API does not allow a single BUILD to have a mix of these though, correct? If you wanted to include dev deps for some targets (such as tests) but not for others (such as libraries).

Would something such as :node_modules[__{dev,prod}] as the API also solve this?

It does allow a mix. You could write npm_link_targets() and npm_link_targets(prod = True) and npm_link_targets(dev = True) in the same BUILD if you wanted.

I was thinking you can't have multiple npm_link_all_packages() targets though...?

Do we need the npm_link_all_packages API changes in this PR at all?

I can't remember why I included the npm_link_all_packages changes originally; I guess just for parallelism with npm_link_targets. We're only using npm_link_targets(prod = True).

npm/extensions.bzl

jbedard · 2025-01-03T20:54:01Z

npm/private/test/snapshots/bzlmod/npm_defs.bzl

@@ -2048,371 +2048,369 @@ def npm_link_all_packages(name = "node_modules", imported_links = []):
    if link:
        if bazel_package == "js/private/worker/src":
            link_2(name = "{}/abortcontroller-polyfill".format(name))
-            link_targets.append("//{}:{}/abortcontroller-polyfill".format(bazel_package, name))
+            link_targets.append("//{}:{}/abortcontroller-polyfill".format(bazel_package, name)) if (not prod and not dev) or (prod and not True) or (dev and True) else None


This generated if ... is ugly and I'm pondering how else we can do it. I think we can generate something simpler.

Can't we just compute a few vars at the top of the method such as include_dev and include_prod? Then this will be if include_dev else None or if include_prod else None or blank (or if True else None instead of blank).

Yeah, something like that should be possible I think.

After taking another look, I don't think it is possible. The contents of this fragment depend on whether the package in question is a production or dev dependency for the bazel_package in question. I.e., it's not a simple include_dev / include_prod boolean for each package, but rather a matrix of bazel_package x link_alias, which we're encoding as a nested conditional.

There might be some other way to structure this, but it would require a more invasive change.

jbedard · 2025-01-03T20:56:30Z

IIUC this solves the issue where rules_js currently treats the lockfile dev flag incorrectly, but does not add the API suggested in #1879?

jfirebaugh · 2025-02-03T22:19:45Z

IIUC this solves the issue where rules_js currently treats the lockfile dev flag incorrectly, but does not add the API suggested in #1879?

This is my suggested solution for #1879. I think it's orthogonal to the lockfile dev flag issue. The determination of dev versus non-dev is done per pnpm workspace member, based on the devDependencies arrays in pnpm-lock.yaml, not the dev flag.

jbedard · 2025-02-05T18:40:30Z

npm/private/transitive_closure.bzl

@@ -115,6 +115,7 @@ def translate_to_transitive_closure(importers, packages, prod = False, dev = Fal
            # deps this importer should pass on if it is linked as a first-party package; this does
            # not include devDependencies
            "deps": deps,
+            "dev_deps": dev_deps,
            # all deps of this importer to link in the node_modules folder of that Bazel package and
            # make available to all build targets; this includes devDependencies
            "all_deps": all_deps,


Is this still used?

No, will remove.

jbedard · 2025-05-01T16:45:24Z

@jfirebaugh have you had a chance to make any updates here? Are you using the patch in your repo?

WDYT of npm_link_all_packages(name = "node_modules") simply producing additional targets instead of accepting the flags?

:node_modules__dev
:node_modules__prod

Or would that add more confusion with things like :node_modules/@types? 🤔

Then npm_link_targets(dev, prod) like you already have here when we're just returning a list of targets.

jfirebaugh · 2025-07-14T23:27:55Z

Still using this in our repo. Just updated the patch for 2.3.8 and removed the change to npm_link_all_packages's API.

jfirebaugh · 2025-07-14T23:31:42Z

Hmm, removing that breaks something for us. Will have to investigate later.

jfirebaugh mentioned this pull request Dec 20, 2024

[FR]: npm_link_all_packages should provide shorthands for "all non-devDependencies" and "all devDependencies" #1879

Open

jfirebaugh force-pushed the jfirebaugh/npm_link_targets branch from e0a193d to 5ef2ba5 Compare December 20, 2024 21:12

jbedard reviewed Jan 3, 2025

View reviewed changes

npm/extensions.bzl Show resolved Hide resolved

jbedard reviewed Jan 3, 2025

View reviewed changes

jbedard mentioned this pull request Jan 3, 2025

[Bug]: dev = True support missing from pnpm version 9 lockfiles #2013

Open

jbedard requested a review from dzbarsky January 8, 2025 23:52

jbedard reviewed Feb 5, 2025

View reviewed changes

feat: add prod and dev attributes to npm_link_targets

ede5b45

jfirebaugh force-pushed the jfirebaugh/npm_link_targets branch 2 times, most recently from ede5b45 to 90378de Compare July 14, 2025 23:26

jfirebaugh force-pushed the jfirebaugh/npm_link_targets branch from 90378de to ede5b45 Compare July 14, 2025 23:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

feat: add `prod` and `dev` attributes to `npm_link_targets` #2051

feat: add `prod` and `dev` attributes to `npm_link_targets` #2051

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat: add prod and dev attributes to npm_link_targets #2051

Are you sure you want to change the base?

feat: add prod and dev attributes to npm_link_targets #2051

Uh oh!

Conversation

Changes are visible to end-users: yes

Test plan

Uh oh!

Uh oh!

e2e/bzlmod

e2e/gyp_no_install_script

e2e/js_image_oci

e2e/npm_link_package

e2e/npm_link_package-esm

e2e/npm_translate_lock

e2e/npm_translate_lock_empty

e2e/npm_translate_lock_multi

e2e/npm_translate_lock_partial_clone

e2e/npm_translate_lock_replace_packages

e2e/npm_translate_lock_subdir_patch

e2e/npm_translate_package_lock

e2e/npm_translate_yarn_lock

e2e/package_json_module

e2e/pnpm_lockfiles

e2e/pnpm_workspace

e2e/pnpm_workspace_rerooted

e2e/repo_mapping

e2e/rules_foo

e2e/runfiles

e2e/vendored_node

Buildifier

Format

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feat: add `prod` and `dev` attributes to `npm_link_targets` #2051

feat: add `prod` and `dev` attributes to `npm_link_targets` #2051