Skip to content

refactor: add simplified package store definition for no-hook non-circular packages #2557

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.

Sign up for GitHub

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

Jump to bottom
Draft
jbedard wants to merge 1 commit into
base: 3.x
Choose a base branch
from
Draft

refactor: add simplified package store definition for no-hook non-circular packages #2557

jbedard wants to merge 1 commit into from

Conversation

@jbedard
Copy link
Member

@jbedard jbedard commented Dec 5, 2025 •
edited
Loading

Add a simplified codepath (npm_imported_simple_package_store_internal) for packages with no circular deps and no lifecycle hooks. While it adds a bit of code it makes the most common scenario much simpler to follow.

Changes are visible to end-users: no

Test plan

  • Covered by existing test cases

@aspect-workflows
Copy link

aspect-workflows bot commented Dec 5, 2025 •
edited
Loading

Bazel 7 (Test)

5 test targets passed

Targets 
//npm/private/test:write_npm_translate_lock_3_test [k8-fastbuild] 189ms
//npm/private/test:write_npm_translate_lock_4_test [k8-fastbuild] 175ms
//npm/private/test:write_npm_translate_lock_5_test [k8-fastbuild] 194ms
//npm/private/test:write_npm_translate_lock_6_test [k8-fastbuild] 202ms
//npm/private/test:write_npm_translate_lock_7_test [k8-fastbuild] 133ms

Total test execution time was 893ms. 278 tests (98.2%) were fully cached saving 36s.

Bazel 8 (Test)

All tests were cache hits

242 tests (100.0%) were fully cached saving 31s.

Bazel 7 (Test)

e2e/bzlmod

All tests were cache hits

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

Bazel 7 (Test)

e2e/git_dep_metadata

All tests were cache hits

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

Bazel 7 (Test)

e2e/gyp_no_install_script

1 test target passed

Targets 
//:write_npm_translate_lock_bzlmod_test [k8-fastbuild]            128ms

Total test execution time was 128ms. 1 test (50.0%) was fully cached saving 59ms.

Bazel 7 (Test)

e2e/js_image_oci

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_link_package

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_link_package-esm

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_link_package-rerooted

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_translate_lock

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_translate_lock_disable_hooks

1 test target passed

Targets 
//:write_npm_translate_lock_defs_1_test [k8-fastbuild]            58ms

Total test execution time was 58ms. 2 tests (66.7%) were fully cached saving 133ms.

Bazel 7 (Test)

e2e/npm_translate_lock_empty

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_translate_lock_exclude_package_contents

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_translate_lock_multi

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_translate_lock_partial_clone

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_translate_lock_replace_packages

All tests were cache hits

4 tests (100.0%) were fully cached saving 366ms.

Bazel 7 (Test)

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_translate_package_lock

All tests were cache hits

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

Bazel 7 (Test)

e2e/npm_translate_yarn_lock

All tests were cache hits

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

Bazel 7 (Test)

e2e/package_json_module

All tests were cache hits

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

Bazel 7 (Test)

e2e/patch_from_repo

All tests were cache hits

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

Bazel 7 (Test)

e2e/pnpm_lockfiles

12 test targets passed

Targets 
//v101:repos_10_test [k8-fastbuild]                               74ms
//v101:repos_11_test [k8-fastbuild]                               95ms
//v101:repos_3_test [k8-fastbuild]                                74ms
//v101:repos_4_test [k8-fastbuild]                                102ms
//v101:repos_5_test [k8-fastbuild]                                62ms
//v101:repos_6_test [k8-fastbuild]                                99ms
//v90:repos_10_test [k8-fastbuild]                                83ms
//v90:repos_11_test [k8-fastbuild]                                84ms
//v90:repos_3_test [k8-fastbuild]                                 77ms
//v90:repos_4_test [k8-fastbuild]                                 82ms
//v90:repos_5_test [k8-fastbuild]                                 59ms
//v90:repos_6_test [k8-fastbuild]                                 104ms

Total test execution time was 995ms. 23 tests (65.7%) were fully cached saving 3s.

Bazel 7 (Test)

e2e/pnpm_repo_install

All tests were cache hits

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

Bazel 7 (Test)

e2e/pnpm_version

All tests were cache hits

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

Bazel 7 (Test)

e2e/pnpm_workspace

All tests were cache hits

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

Bazel 7 (Test)

e2e/pnpm_workspace_deps

All tests were cache hits

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

Bazel 7 (Test)

e2e/pnpm_workspace_rerooted

All tests were cache hits

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

Bazel 7 (Test)

e2e/repo_mapping

All tests were cache hits

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

Bazel 7 (Test)

e2e/runfiles

All tests were cache hits

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

Bazel 7 (Test)

e2e/stamped_package_json

All tests were cache hits

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

Bazel 7 (Test)

e2e/vendored_node

All tests were cache hits

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

Bazel 7 (Test)

e2e/vendored_tarfile

All tests were cache hits

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

Bazel 7 (Test)

e2e/verify_patches

All tests were cache hits

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

Bazel 7 (Test)

e2e/worker

All tests were cache hits

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

Buildifier      Format

@jbedard jbedard force-pushed the fork-complicated-pkgs branch 3 times, most recently from be977b9 to bd92bcb Compare December 5, 2025 20:24
@jbedard jbedard changed the title refactor: distinguish handling of third-party package lifecycle hooks vs circular deps refactor: only use npm_import(transitive_closure) for circular deps Dec 5, 2025
jbedard
jbedard commented Dec 5, 2025
View reviewed changes
npm/private/npm_import.bzl Outdated
)

# reference target used when referenced by a package with cycles
_npm_package_store(
Copy link
Member Author

@jbedard jbedard Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One reason to create npm_imported_simple_package_store_internal... if we can determine that this package is never part of a circular dep then /ref and /pkg can either be omitted or maybe alias()s to the main target (we do aliases for local package store entries already - or we can have a breaking change to no longer guarantee these targets exist).

That will be a followup...

@jbedard jbedard force-pushed the fork-complicated-pkgs branch from bd92bcb to d6599b4 Compare December 5, 2025 21:37
@jbedard jbedard changed the title refactor: only use npm_import(transitive_closure) for circular deps refactor: add simplified package store definition for no-hook non-circular packages Dec 5, 2025
jbedard
jbedard commented Dec 5, 2025
View reviewed changes
npm/private/npm_import.bzl
exclude_package_contents):
bazel_package = native.package_name()
is_root = bazel_package == root_package
if not is_root:
Copy link
Member Author

@jbedard jbedard Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This macro is only invoked from generated code where we already checked this, so I'm deleting it instead of duplicating it again into the new macro.

@jbedard jbedard force-pushed the fork-complicated-pkgs branch from d6599b4 to d5cb56d Compare December 6, 2025 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jbedard