Skip to content

fix: do not use npm_translate_lock patch_args without patches#1693

Closed
jbedard wants to merge 1 commit into
aspect-build:mainfrom
jbedard:1427-pnpm-patch-args
Closed

fix: do not use npm_translate_lock patch_args without patches#1693
jbedard wants to merge 1 commit into
aspect-build:mainfrom
jbedard:1427-pnpm-patch-args

Conversation

@jbedard

@jbedard jbedard commented May 3, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Fix #1427
An alternative to #837 from @kormide

This makes pure pnpm.patchedDependencies nicer like #837, normally having the same effect, I think?

I think the main difference is that #837 removes -p0 if there are no npm_translate_lock(patches) but still applies the rest of npm_translate_lock(patch_args). While this PR does not apply npm_translate_lock(patch_args) when there are no npm_translate_lock(patches).

Maybe this PR is actually more confusing? 🤦
Maybe we should just drop support for mixing these 2 types of patches in 2.x? See #1694

Test plan

  • Covered by existing test cases
  • Manual testing; please provide instructions so we can reproduce:

@aspect-workflows

aspect-workflows Bot commented May 3, 2024 •
edited
Loading

Copy link
Copy Markdown

Test

All tests were cache hits

211 tests (100.0%) were fully cached saving 42s.

Test

e2e/bzlmod

All tests were cache hits

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

Test

e2e/gyp_no_install_script

All tests were cache hits

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

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

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

Test

e2e/npm_link_package-esm

All tests were cache hits

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

Test

e2e/npm_translate_lock

All tests were cache hits

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

Test

e2e/npm_translate_lock_empty

All tests were cache hits

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

Test

e2e/npm_translate_lock_multi

All tests were cache hits

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

Test

e2e/npm_translate_lock_partial_clone

All tests were cache hits

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

Test

e2e/npm_translate_lock_subdir_patch

All tests were cache hits

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

Test

e2e/npm_translate_package_lock

All tests were cache hits

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

Test

e2e/npm_translate_yarn_lock

All tests were cache hits

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

Test

e2e/package_json_module

All tests were cache hits

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

Test

e2e/pnpm_lockfiles

All tests were cache hits

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

Test

e2e/pnpm_workspace

All tests were cache hits

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

Test

e2e/pnpm_workspace_rerooted

All tests were cache hits

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

Test

e2e/repo_mapping

All tests were cache hits

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

Test

e2e/rules_foo

All tests were cache hits

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

Test

e2e/vendored_node

All tests were cache hits

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

Buildifier      Format

@jbedard jbedard mentioned this pull request May 3, 2024
Merged
@jbedard

jbedard commented May 3, 2024

Copy link
Copy Markdown
Member Author

I think we should just do #1694 instead of this or #837

WDYT?

jbedard added a commit that referenced this pull request May 9, 2024
@jbedard
fix: do not support mixing pnpm.patchedDependencies and npm_translate…
b66dad1 
…_lock(patches) (#1694)

Fix #1427
Close #1693
Close #837
@jbedard

jbedard commented May 9, 2024

Copy link
Copy Markdown
Member Author

Done with #1694

@jbedard jbedard closed this May 9, 2024
@jbedard jbedard deleted the 1427-pnpm-patch-args branch May 9, 2024 18:58
gregmagolan pushed a commit that referenced this pull request May 10, 2024
@jbedard
fix: do not support mixing pnpm.patchedDependencies and npm_translate…
f045423 
…_lock(patches) (#1694)

Fix #1427
Close #1693
Close #837
gregmagolan pushed a commit that referenced this pull request May 10, 2024
@jbedard
fix: do not support mixing pnpm.patchedDependencies and npm_translate…
29b9afd 
…_lock(patches) (#1694)

Fix #1427
Close #1693
Close #837
gregmagolan pushed a commit that referenced this pull request May 13, 2024
@jbedard
fix: do not support mixing pnpm.patchedDependencies and npm_translate…
96902e9 
…_lock(patches) (#1694)

Fix #1427
Close #1693
Close #837
jbedard added a commit to jbedard/rules_js that referenced this pull request May 14, 2024
@jbedard
fix: do not support mixing pnpm.patchedDependencies and npm_translate…
8abac39 
…_lock(patches) (aspect-build#1694)

Fix aspect-build#1427
Close aspect-build#1693
Close aspect-build#837
jbedard added a commit that referenced this pull request May 14, 2024
@jbedard
fix: do not support mixing pnpm.patchedDependencies and npm_translate…
6dd0376 
…_lock(patches) (#1694)

Fix #1427
Close #1693
Close #837
jbedard added a commit to jbedard/rules_js that referenced this pull request May 16, 2024
@jbedard
fix: do not support mixing pnpm.patchedDependencies and npm_translate…
68e1333 
…_lock(patches) (aspect-build#1694)

Fix aspect-build#1427
Close aspect-build#1693
Close aspect-build#837
gregmagolan pushed a commit that referenced this pull request May 20, 2024
@jbedard
fix: do not support mixing pnpm.patchedDependencies and npm_translate…
f43be0b 
…_lock(patches) (#1694)

Fix #1427
Close #1693
Close #837
gregmagolan pushed a commit that referenced this pull request May 20, 2024
@jbedard
fix: do not support mixing pnpm.patchedDependencies and npm_translate…
bfb2293 
…_lock(patches) (#1694)

Fix #1427
Close #1693
Close #837
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gregmagolan gregmagolan Awaiting requested review from gregmagolan

@kormide kormide Awaiting requested review from kormide

@thesayyn thesayyn Awaiting requested review from thesayyn

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Error from npm_translate_lock when using pnpm.patchedDependencies

1 participant

@jbedard