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

⚠️ Buildkite build #6877 failed.

Failed tests (2)

//npm/private:_test_gendocs_0_0 [k8-fastbuild]                🔗
//npm/private:_test_gendocs_0_1 [k8-fastbuild]                🔗

💡 To reproduce the test failures, run

bazel test //npm/private:_test_gendocs_0_1 //npm/private:_test_gendocs_0_0

Test

e2e/bzlmod

All tests were cache hits

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

Test

e2e/gyp_no_install_script

All tests were cache hits

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

Test

e2e/js_image_oci

All tests were cache hits

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

Test

e2e/npm_link_package

All tests were cache hits

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

Test

e2e/npm_link_package-esm

All tests were cache hits

3 tests (100.0%) were fully cached saving 1s.

Test

e2e/npm_translate_lock

All tests were cache hits

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

Test

e2e/npm_translate_lock_empty

All tests were cache hits

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

Test

e2e/npm_translate_lock_multi

All tests were cache hits

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

Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

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

Test

e2e/npm_translate_lock_replace_packages

⚠️ Buildkite build #6877 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 222ms.

Test

e2e/npm_translate_package_lock

All tests were cache hits

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

Test

e2e/npm_translate_yarn_lock

All tests were cache hits

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

Test

e2e/package_json_module

All tests were cache hits

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

Test

e2e/pnpm_lockfiles

⚠️ Buildkite build #6877 failed.

Failed tests (4)

//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 //v54:repos_0_test //v90:repos_0_test //v61:repos_0_test //v60:repos_0_test

Test

e2e/pnpm_workspace

All tests were cache hits

10 tests (100.0%) were fully cached saving 3s.

Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

12 tests (100.0%) were fully cached saving 2s.

Test

e2e/repo_mapping

All tests were cache hits

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

Test

e2e/rules_foo

All tests were cache hits

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

Test

e2e/runfiles

All tests were cache hits

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

Test

e2e/vendored_node

All tests were cache hits

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

Buildifier

Format

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...?

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

npm/extensions.bzl

@@ -134,6 +134,9 @@ WARNING: Cannot determine home directory in order to load home `.npmrc` file in
    system_tar = detect_system_tar(module_ctx)

    for i in imports:
+        link_packages = {}
+        for link_package, link_names in i.link_packages.items():
+            link_packages[link_package] = [link_name["pkg"] for link_name in link_names]
        npm_import(


(this is probably beyond this scope of this PR, but I'd like to start the discussion here...)

I think the npm_import(dev) flag is flawed and should actually be removed completely. The dev vs prod logic needs to be 100% at package link time based on the package.json dev vs non-dev dependencies, not the lockfile dev flag which is currently misused in rules_js (and non-existent in pnpm9).

Does that sound right to you?

I agree that the behavior of lockfile dev flag makes it unsuitable for really anything that rules_js wants to do with regard to dev dependencies.

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.

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.

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

feat: add prod and dev attributes to npm_link_targets

5ef2ba5

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jfirebaugh commented Dec 20, 2024

aspect-workflows bot commented Dec 20, 2024 •

edited

Loading

jbedard Jan 3, 2025

jbedard Jan 3, 2025

jfirebaugh Feb 3, 2025

jbedard Feb 3, 2025

jfirebaugh Feb 4, 2025 •

edited

Loading

jbedard Feb 4, 2025

jbedard Jan 3, 2025

jfirebaugh Feb 3, 2025

jbedard Jan 3, 2025

jfirebaugh Feb 3, 2025

jbedard commented Jan 3, 2025

jfirebaugh commented Feb 3, 2025 •

edited

Loading

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

Conversation

jfirebaugh commented Dec 20, 2024

Changes are visible to end-users: yes

Test plan

aspect-workflows bot commented Dec 20, 2024 • edited Loading

Test

Test

e2e/bzlmod

Test

e2e/gyp_no_install_script

Test

e2e/js_image_oci

Test

e2e/npm_link_package

Test

e2e/npm_link_package-esm

Test

e2e/npm_translate_lock

Test

e2e/npm_translate_lock_empty

Test

e2e/npm_translate_lock_multi

Test

e2e/npm_translate_lock_partial_clone

Test

e2e/npm_translate_lock_replace_packages

Test

e2e/npm_translate_lock_subdir_patch

Test

e2e/npm_translate_package_lock

Test

e2e/npm_translate_yarn_lock

Test

e2e/package_json_module

Test

e2e/pnpm_lockfiles

Test

e2e/pnpm_workspace

Test

e2e/pnpm_workspace_rerooted

Test

e2e/repo_mapping

Test

e2e/rules_foo

Test

e2e/runfiles

Test

e2e/vendored_node

Buildifier

Format

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfirebaugh Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbedard commented Jan 3, 2025

jfirebaugh commented Feb 3, 2025 • edited Loading

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

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

aspect-workflows bot commented Dec 20, 2024 •

edited

Loading

jfirebaugh Feb 4, 2025 •

edited

Loading

jfirebaugh commented Feb 3, 2025 •

edited

Loading