[Bug]: rules_js 2.0 pnpm 9 fails to load pnpm-lock.yaml

Question

[Bug]: rules_js 2.0 pnpm 9 fails to load pnpm-lock.yaml

hjellek opened this issue 6 months ago · 19 comments

What happened?

Thanks for all your hard work!

After upgrading our monorepo to rules_js 2.0.0-alpha.7 and testing the pnpm9 support, npm_translate_lock seems to have problem parsing:

ERROR: Traceback (most recent call last):
	File "/private/var/tmp/_bazel_knut.hjelle/5c024f616e5754a9bdc69a64d7cdfdce/external/aspect_rules_js~/npm/extensions.bzl", line 29, column 41, in _npm_extension_impl
		_npm_lock_imports_bzlmod(module_ctx, attr)
	File "/private/var/tmp/_bazel_knut.hjelle/5c024f616e5754a9bdc69a64d7cdfdce/external/aspect_rules_js~/npm/extensions.bzl", line 83, column 58, in _npm_lock_imports_bzlmod
		importers, packages = translate_to_transitive_closure(
	File "/private/var/tmp/_bazel_knut.hjelle/5c024f616e5754a9bdc69a64d7cdfdce/external/aspect_rules_js~/npm/private/transitive_closure.bzl", line 197, column 71, in translate_to_transitive_closure
		package_info["transitive_closure"] = gather_transitive_closure(
	File "/private/var/tmp/_bazel_knut.hjelle/5c024f616e5754a9bdc69a64d7cdfdce/external/aspect_rules_js~/npm/private/transitive_closure.bzl", line 67, column 21, in gather_transitive_closure
		fail(msg)
Error in fail: Unknown package key: @docusaurus/react-loadable@5.5.2_react_18.2.0 in ["@algolia/autocomplete-core/1.9.3_755392440", .....

I see that the package is listed in the list with a slightly different format:

"@docusaurus/react-loadable/5.5.2_react_18.2.0"

vs

@docusaurus/react-loadable@5.5.2_react_18.2.0

I created a demo-branch in a separate repository that also shows the same error here: https://github.com/hjellek/rules_js_alpha_test/compare/pnpm_9

Version

Development (host) and target OS/architectures:
macos, arm64

Output of bazel --version:
bazel 7.1.2

Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file:
rules_js 2.0.0-alpha.7

Language(s) and/or frameworks involved:
JavaScript, Docusaurus

How to reproduce

I created a repo which reproduces the problem in the pnpm_9 branch
https://github.com/hjellek/rules_js_alpha_test

On `main`, `bazel build //apps/docusaurus:app --spawn_strategy=local` works as expected (docusaurus currently hangs on sandbox)

On the branch `pnpm_9` where the lockfile format is updated to v9.0 (updated with `bazel run -- @pnpm//:pnpm --dir "$PWD" i --lockfile-only`), build fails due to missing packages while resolving the lockfile with the error listed

Any other information?

No response

Answer 1 · 2024-05-21T14:56:50.000Z

Thanks for trying out the alpha release and the bug report. @jbedard is currently working on a refactor on the lock file parsing code (#1735) that will hopefully fix this. The changes in the pnpm v9 lockfile format were significant enough that a refactor and better test coverage was warranted. After that lands we'll cut another alpha release.

Answer 2 · 2024-05-21T17:56:55.000Z

@hjellek would you be able to try again in the next alpha after #1735 merges? That fixes some edge cases with pnpm9 which might include this issue.

Answer 3 · 2024-05-21T20:00:57.000Z

@hjellek Can you try again with https://github.com/aspect-build/rules_js/releases/tag/v2.0.0-alpha.8?

Answer 4 · 2024-05-22T06:24:28.000Z

@gregmagolan / @jbedard I've tried with 2.0.0-rc0 and now it seems to work as expected! Thanks for handling it so quickly.

Answer 5 · 2024-05-22T06:42:35.000Z

Great 👍

I'll close this for now then. But please log another or comment here if you see anything similar.

Answer 6 · 2024-05-22T06:44:13.000Z

But I might just have found another and probably related bug.

In our main repo, npm_translate_lock now resolves just fine. But one of our applications running Docusaurus fails building due to:

Module not found: Error: Can't resolve 'react-loadable' in '/private/var/tmp/_bazel_knut.hjelle/e8e018c965c1b1ff18d8534c832243ca/execroot/_main/bazel-out/darwin_arm64-fastbuild/bin/node_modules/.aspect_rules_js/@docusaurus+core@3.1.1_-122611188/node_modules/@docusaurus/core/lib/client/exports'

I see that react-loadable is listed in @docusaurus/core as "react-loadable": "npm:@docusaurus/react-loadable@5.5.2",

bazel-out/darwin_arm64-fastbuild/bin/node_modules/.aspect_rules_js/@docusaurus+react-loadable@5.5.2_react_18.2.0 exists, but not bazel-out/darwin_arm64-fastbuild/bin/node_modules/.aspect_rules_js/react-loadable@5.5.2_react_18.2.0 which I guess is what Docusaurus expects.

I don't see the same error in the example repo but that might be because the app itself is empty - I will try to reproduce it there as well.

Answer 7 · 2024-05-22T07:52:35.000Z

Yeah, that sounds related to pnpm9 and how those deps are processed in rules_js.

I was having issues with npm: even with plain pnpm9 though so I haven't yet added proper tests for it yet within rules_js either.

If you can put a minimal repro together that would be great 👍

Answer 8 · 2024-05-22T08:48:57.000Z

I have updated the case in https://github.com/hjellek/rules_js_alpha_test/compare/pnpm_9 now.

It did not fail until I ran bazel clean first, probably leftover symlinks from the pnpm 8 install locally on my machine.

Answer 9 · 2024-05-22T17:16:03.000Z

@hjellek I cloned down your repo to try to repro and the build hung indefinitely on the first bazel build:

DEBUG: /private/var/tmp/_bazel_greg/35c5b8f055f6df6a32439a05008ada9c/external/aspect_rules_js~/npm/extensions.bzl:272:18: NOTE: repo 'pnpm' has multiple versions ["9.1.1", "8.6.7"]; selected 9.1.1
INFO: Analyzed 6 targets (1184 packages loaded, 12675 targets configured).
[14,505 / 14,506] Docusaurus apps/docusaurus/build; 6721s darwin-sandbox

Did you ever observe that in your testing?

Answer 10 · 2024-05-23T05:58:06.000Z

@gregmagolan yeah, there is something probably traversing symlinks in one of the webpack plugins for docusaurus - I have never been able to find it unfortunately. I mentioned it in how to reproduce, as we build it with --spawn_strategy=local

Answer 11 · 2024-05-23T06:38:07.000Z

I think I managed to reproduce it with a more local setup in the branch pnpm_9_npm_rename_package here: hjellek/rules_js_alpha_test@pnpm_9...pnpm_9_npm_rename_package

ERROR: /[....]/rules_js_alpha_test/BUILD.bazel:3:22: in deps attribute of npm_package_store rule //:.aspect_rules_js/node_modules/package-dep-override@0.0.0: target '//:.aspect_rules_js/node_modules/local-lpad@lpad@3.0.0' does not exist. Since this rule was created by the macro 'npm_link_all_packages', the error might have been caused by the macro implementation

lpad seems to be correctly synced up, but local-lpad is not

bazel query //... | grep lpad
Loading: 0 packages loaded
//:.aspect_rules_js/node_modules/lpad@3.0.0
//:.aspect_rules_js/node_modules/lpad@3.0.0/dir
//:.aspect_rules_js/node_modules/lpad@3.0.0/pkg
//:.aspect_rules_js/node_modules/lpad@3.0.0/ref
//apps/package-dep-override:node_modules/local-lpad
//apps/package-dep-override:node_modules/local-lpad/dir

Answer 12 · 2024-05-23T17:45:05.000Z

The npm: case is one I'm working on atm. I've listed the known issues currently being worked on in #1753

Answer 13 · 2024-06-02T19:13:21.000Z

@jbedard I think I have stumbled across another edge case with 2.0.0-rc2 😬
Our Remix and NextJS apps fail with a similiar dependency error:

ERROR: /..../rules_js_alpha_test/BUILD.bazel:3:22: in deps attribute of npm_package_store_internal rule //:.aspect_rules_js/node_modules/@isaacs+cliui@8.0.2/pkg: target '//:.aspect_rules_js/node_modules/string-width-cjs@string-width@4.2.3/ref' does not exist. Since this rule was created by the macro 'npm_link_all_packages', the error might have been caused by the macro implementation

I have recreated the problem in https://github.com/hjellek/rules_js_alpha_test/tree/pnpm_9_remix, it should be verifiable with bazel build //apps/remix:build

The package in question lists the same dependency in different versions using the npm: rename strategy https://github.com/isaacs/cliui/blob/main/package.json#L53 but I noticed that the entry in the lockfile does not contain the npm: prefix:

  '@isaacs/cliui@8.0.2':
    dependencies:
      string-width: 5.1.2
      string-width-cjs: string-width@4.2.3

Not sure if that is related though.

Please let me know it you want me to file this as a separate issue, and again - thanks for your work!

Answer 14 · 2024-06-02T22:33:36.000Z

That same package keeps coming up as an interesting test case, and I swear I fixed it. I'll take another look. Thanks for the info 👍

Answer 15 · 2024-06-03T23:45:03.000Z

See 3e08f8e and the latest rc

Answer 16 · 2024-06-04T05:41:11.000Z

@jbedard the remix-example now works as expected with 2.0.0-rc3!

Unfortunately I am back to what looks close to the original error for the docusaurus example I provided:

Error in fail: Unknown package key: @docusaurus/react-loadable@5.5.2(react@18.2.0) (@docusaurus/react-loadable@5.5.2(react @ 18.2.0)) in [...,"@docusaurus/react-loadable@5.5.2_react_18.2.0",...]

See https://github.com/hjellek/rules_js_alpha_test/tree/pnpm_9 and bazel build //apps/docusaurus:all --spawn_strategy=local

This example builds as expected with 2.0.0-rc2.

Answer 17 · 2024-06-04T06:02:53.000Z

I updated the main branch in my example repo with all of the testcases so far.

All except remix builds in 2.0.0-rc2, but after upgrading to 2.0.0-rc3 the lock translation fails as mentioned above.

Answer 18 · 2024-06-04T17:37:14.000Z

Thank you once again @hjellek. I think this fixes it: #1776

More tests every time to (I hope) not break these things in the future :|

Answer 19 · 2024-06-04T20:50:14.000Z

Great work, I will test it when the next rc lands!