Skip to content
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.

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

feat: add support for linking js_library as 1p npm deps #1646

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

gregmagolan
Copy link
Member

@gregmagolan gregmagolan commented Apr 13, 2024

Fixes #1092.

This PR adds the ability to link js_library targets at 1p deps in addition to npm_package targets. Using js_library to link results in a symlink into the js_library package directory in the output tree instead of a copy_to_directory of the npm_package TreeArtifact. This more closely matches how pnpm handles 1p linked deps and it fixes the long standing bug with 1p peer deps reported in #1092.

The new test case //examples/npm_deps:test9 illustrates how the //examples/npm_package/packages/pkg_d:pkg_d js_library that is linked examples/npm_deps/package.json as a "@mycorp/pkg-d": "workspace:*" 1p npm package in the pnpm lock file.

bazel-bin/examples/npm_deps/node_modules/@mycorp/pkg-d -> ../../../../node_modules/.aspect_rules_js/@[email protected]/node_modules/@mycorp/pkg-d

bazel-bin/node_modules/.aspect_rules_js/@[email protected]/node_modules/@mycorp/pkg-d -> ../../../../../examples/npm_package/packages/pkg_d

This is one extra level of indirection vs. what pnpm does but the result is the same in that the npm package 1p symlink examples/npm_deps/node_modules/@mycorp/pkg-d can be resolved back to the output-tree package of the js_library target examples/npm_package/packages/pkg_d. In pnpm, the symlinks goes directly there and not through the package store and, of course, it points into the source tree since there is no concept of an bazel output tree outside of bazel. Under rules_js the extra level of indirection fits nicer into the current support for npm_package and so makes for simpler code to support both modes.


Type of change

  • New feature or functionality (change which adds functionality)

Test plan

  • New test cases added

@jbedard
Copy link
Member

jbedard commented Apr 14, 2024

bazel-bin/node_modules/.aspect_rules_js/@myCorp[email protected]/node_modules/@mycorp/pkg-d -> ../../../../../examples/npm_package/packages/pkg_d

Doesn't that expose the entire packages/pkg_d directory? What if we have 2 js_library in there that both end up in the sandbox... won't they be able to access eachother, the package can access files outside the package etc?

js/private/js_library.bzl Outdated Show resolved Hide resolved
@gregmagolan gregmagolan force-pushed the FR_link_js_library branch 2 times, most recently from 90bb9c8 to 90ea1bc Compare April 15, 2024 16:26
@gregmagolan gregmagolan requested a review from jbedard April 15, 2024 16:27
@gregmagolan gregmagolan mentioned this pull request Apr 29, 2024
21 tasks
jbedard pushed a commit to jbedard/rules_js that referenced this pull request May 14, 2024
jbedard pushed a commit to jbedard/rules_js that referenced this pull request May 16, 2024
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.

2 participants