Skip to content

Conversation

f-luo
Copy link

@f-luo f-luo commented Sep 6, 2025

Addresses issue: #2343

Changes are visible to end-users: yes

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

Test plan

  • Covered by existing test cases

@CLAassistant
Copy link

CLAassistant commented Sep 6, 2025

CLA assistant check
All committers have signed the CLA.

Copy link

aspect-workflows bot commented Sep 6, 2025

Test

All tests were cache hits

222 tests (100.0%) were fully cached saving 30s.


Test

e2e/bzlmod

All tests were cache hits

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


Test

e2e/gyp_no_install_script

All tests were cache hits

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


Test

e2e/js_image_oci

All tests were cache hits

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


Test

e2e/npm_link_package

All tests were cache hits

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


Test

e2e/npm_link_package-esm

All tests were cache hits

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


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 137ms.


Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

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


Test

e2e/npm_translate_lock_replace_packages

All tests were cache hits

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


Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

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


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 301ms.


Test

e2e/pnpm_lockfiles

All tests were cache hits

67 tests (100.0%) were fully cached saving 8s.


Test

e2e/pnpm_workspace

All tests were cache hits

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


Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

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


Test

e2e/repo_mapping

All tests were cache hits

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


Test

e2e/rules_foo

All tests were cache hits

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


Test

e2e/runfiles

All tests were cache hits

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


Test

e2e/vendored_node

All tests were cache hits

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


Buildifier      Format

@matthewjh
Copy link

@f-luo could you try prefixing the chdir value with this in js_binary itself?
https://github.com/aspect-build/rules_js/blob/main/js/private/js_binary.bzl

That would then close #2275.

@f-luo
Copy link
Author

f-luo commented Sep 10, 2025

@f-luo could you try prefixing the chdir value with this in js_binary itself? https://github.com/aspect-build/rules_js/blob/main/js/private/js_binary.bzl

That would then close #2275.

I do think that would address it more broadly. My only concern is that doing it at the js_binary level would risk breaking existing usages, more specifically: if someone is already passing in external/workspace+/pkg.

Unless we also detect the presence of external/...+/ to deduplicate.

Perhaps someone from rules_js (@alexeagle) and the original creator of nextjs_* rulesets (@jbedard) can chime in here?

@alexeagle
Copy link
Member

I agree this should be changed in js_binary. I don't think there are any existing functional use cases that would be broken by stripping external/ from paths.

@f-luo
Copy link
Author

f-luo commented Sep 18, 2025

Okay, that sounds good. I can take a stab at it. But as an FYI I will be traveling for the next 2 weeks, so will come back to this afterwards... unless anyone else would like to drive it in the meantime :)

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

Successfully merging this pull request may close these issues.

4 participants