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

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

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.

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?

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

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

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 Feb 5, 2025

jbedard Jan 3, 2025

jfirebaugh Feb 3, 2025

jbedard commented Jan 3, 2025

jfirebaugh commented Feb 3, 2025 •

edited

Loading

jbedard Feb 5, 2025

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

jbedard commented Jan 3, 2025

jfirebaugh commented Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

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