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

[FR]: link outside WORKSPACE for dependencies during local development (~= pnpm link) #1165

Open
sjbarag opened this issue Jul 17, 2023 · 10 comments
Labels
enhancement New feature or request help wanted Aspect isn't prioritizing this, but the community could

Comments

@sjbarag
Copy link

sjbarag commented Jul 17, 2023

What is the current behavior?

Currently, rules_js fetches packages from either:

  1. A remote location (https://registry.nodejs.org/..., git://github.com/...) with the Bazel downloader
  2. The user workspace (//pkg/left-pad), which requires the package to be pulled into the workspace and its dependencies readable by npm_translate_lock.

Attempts to link outside the Bazel workspace to an NPM package located elsewhere on-disk get rejected with
invalid link_package outside of the WORKSPACE.

Describe the feature

The npm link workflow is pretty common in the node community, as it allows a dev to modify a third-party package and use it without having to vendor that package.

As a contrived example motivation (replace left-pad with any other package):

  1. I've found a pathological case in left-pad performance that my application highlights
  2. I'd like to contribute a fix back to upstream left-pad. While working on that fix, I want to use the locally-changed copy in my application as a sort of "real world" gut-check in addition to the left-pad test suite. Just to make sure my pathological case is indeed getting fixed.
  3. I don't want to vendor left-pad for any number of reasons:
    1. left-pad has a complicated/obscure build system that makes it non-trivial to vendor and build with rules_js.
    2. left-pad takes bugfixes incredibly fast, so my vendored copy would only exist for a few hours after my PR goes out.

In the current state, there appears to be no way to build left-pad in a separate directory and use it in my rules_js-built package(s).

With vanilla pnpm link (or npm link or …), the workflow is roughly:

  1. pnpm --dir ~/bitbucket.org/my-org/my-app link ~/github.com/left-pad/left-pad to use a custom left-pad
  2. Change and rebuild left-pad.
  3. Change and rebuild my-app. It includes my left-pad changes.

Ideally, the workflow would be pretty similar under Bazel + rules_js. For local development it feels reasonable to break hermeticity, akin to npm_translate_lock's current use_home_npmrc), but I'd be happy to add an extra flag or two to achieve this.


Relevant slack conversation: https://bazelbuild.slack.com/archives/CEZUUKQ6P/p1689358470077099

@sjbarag sjbarag added the enhancement New feature or request label Jul 17, 2023
@sjbarag
Copy link
Author

sjbarag commented Jul 17, 2023

For a non-contrived motivation, included here to avoid cluttering the main description:

  1. My large Bazel-built open-source application $app has a major release every six months.
  2. That application shares UI components with a closed-source hosted webapp.
  3. That hosted webapp supports multiple major-versions of the OSS $app. I'd like those shared UI components to be versioned independently of the OSS $app, such that the shared UI components themselves support multiple major versions of $app.
  4. Keeping those shared components in a separate repo with separate history feels the most natural to achieve that, but makes local development of $app and the shared UI components very difficult.

@sjbarag sjbarag changed the title [FR]: link outside WORKSPACE for dependencies on-disk (~= pnpm link) [FR]: link outside WORKSPACE for dependencies during local development (~= pnpm link) Jul 17, 2023
@github-actions github-actions bot added the untriaged Requires traige label Jul 17, 2023
@twheys
Copy link

twheys commented Jul 18, 2023

If you link a package link pnpm link lodash, the dependencies in the pnpm.lock file get modified to something like

      lodash:
        specifier: 4.17.4
        version: link:node_modules/lodash

However, rules_js doesn't like this and errors out

ERROR: .../BUILD.bazel:6:22: no such package 'node_modules/lodash': Package is considered deleted due to --deleted_packages and referenced by '//:.aspect_rules_js/node_modules/[email protected]'

Seems like this could work hermetically since linking changes the lock file. Would be really nice if we could get support for this feature, since it's a very common workflow for developers who maintain 3rd party packages in their workspaces. I'd be happy to make a PR if someone from aspect could give me a little guidance of how this might be implemented. Please let me know.

sjbarag added a commit to sjbarag/cockroach that referenced this issue Jul 18, 2023
When working with first-party JS dependencies that aren't in this
monorepo, the idiomatic development workflow uses pnpm link [1] to link
multiple JS packages together. Specifically, running
'pnpm link /path/to/github.com/cockroachdb/ui/packages/foo' from within
pkg/ui/workspaces/* creates a symbolic link at
node_modules/@cockroachlabs/foo that points to
../../(…)/ui/packages/foo. This works quite smoothly for local
development, as changes in the 'foo' package are automatically seen by a
'pnpm webpack --watch' running in CRDB. The two packages act like
they're maintained in the same repo, while allowing independent version
history, CI processes, etc.

Unfortunately, rules_js currently offers no way to link outside of the
Bazel workspace. Such a symlink is either ignored by rules_js (since it
doesn't appear in pnpm-lock.yaml) or is rejected if it's manually added
to the lockfile [2]. Allowing Bazel-based builds of CockroachDB when
'pnpm link'ed packages are present introduces dependency skew between
Bazel and non-Bazel builds. To allow 'pnpm link' to be used safely,
pre-emptively reject builds of 'cockroach' and 'cockroach-oss' that are
run through the 'dev' helper when linked @cockroachlabs/ packages are
detected.

[1] https://pnpm.io/cli/link
[2] aspect-build/rules_js#1165

Release note: None
Epic: none
sjbarag added a commit to sjbarag/cockroach that referenced this issue Jul 19, 2023
When working with first-party JS dependencies that aren't in this
monorepo, the idiomatic development workflow uses pnpm link [1] to link
multiple JS packages together. Specifically, running
'pnpm link /path/to/github.com/cockroachdb/ui/packages/foo' from within
pkg/ui/workspaces/* creates a symbolic link at
node_modules/@cockroachlabs/foo that points to
../../(…)/ui/packages/foo. This works quite smoothly for local
development, as changes in the 'foo' package are automatically seen by a
'pnpm webpack --watch' running in CRDB. The two packages act like
they're maintained in the same repo, while allowing independent version
history, CI processes, etc.

Unfortunately, rules_js currently offers no way to link outside of the
Bazel workspace. Such a symlink is either ignored by rules_js (since it
doesn't appear in pnpm-lock.yaml) or is rejected if it's manually added
to the lockfile [2]. Allowing Bazel-based builds of CockroachDB when
'pnpm link'ed packages are present introduces dependency skew between
Bazel and non-Bazel builds. To allow 'pnpm link' to be used safely,
pre-emptively reject builds of 'cockroach' and 'cockroach-oss' that are
run through the 'dev' helper when linked @cockroachlabs/ packages are
detected.

[1] https://pnpm.io/cli/link
[2] aspect-build/rules_js#1165

Release note: None
Epic: none
craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Jul 19, 2023
107110: kvserver: avoid running intensive decommission test under deadlock r=AlexTalks a=AlexTalks

The kvserver test `TestDecommission`, which runs a 5-node cluster and decommissions 4 of those 5 nodes, has trouble completing fast enough when under a race or deadlock configuration. While race configurations were already skipped, this modifies the test to be skipped under deadlock configurations as well.

Fixes: #106096

Release note: None

107111: compose: start `docker-compose` with a non-empty `PATH` r=rail a=rickystewart

`docker-compose` invokes `docker`, but obviously this will fail if there is nothing in the `PATH`.

Epic: none
Release note: None

107129: dev: reject builds when CRL JS dependencies are 'pnpm link'ed r=rickystewart a=sjbarag

When working with first-party JS dependencies that aren't in this monorepo, the idiomatic development workflow uses pnpm link [1] to link multiple JS packages together. Specifically, running `pnpm link /path/to/github.com/cockroachdb/ui/packages/foo` from within pkg/ui/workspaces/* creates a symbolic link at
node_modules/`@cockroachlabs/foo` that points to
../../(…)/ui/packages/foo. This works quite smoothly for local development, as changes in the 'foo' package are automatically seen by a `pnpm webpack --watch` running in CRDB. The two packages act like they're maintained in the same repo, while allowing independent version history, CI processes, etc.

Unfortunately, rules_js currently offers no way to link outside of the Bazel workspace. Such a symlink is either ignored by rules_js (since it doesn't appear in pnpm-lock.yaml) or is rejected if it's manually added to the lockfile [2]. Allowing Bazel-based builds of CockroachDB when 'pnpm link'ed packages are present introduces dependency skew between Bazel and non-Bazel builds. To allow `pnpm link` to be used safely, pre-emptively reject builds of 'cockroach' and 'cockroach-oss' that are run through the 'dev' helper when linked `@cockroachlabs/` packages are detected.

[1] https://pnpm.io/cli/link
[2] aspect-build/rules_js#1165

Release note: None
Epic: none

-----

Example output: 
<img width="998" alt="image" src="https://github.com/cockroachdb/cockroach/assets/665775/3fd43abe-f5c2-4ddd-bc60-16a73db12836">

Total duration is 16ms on my machine since we're checking so few files. We can drop these into a goroutine per first-party JS if we want, but this is certainly fast enough to be conversational.

107149: opt: make functional dependency calculation deterministic r=DrewKimball a=DrewKimball

We recently added additional logic for inferring functional dependencies for join expressions. However, this logic iterates through a map, which leads to nondeterminism in which order functional dependencies are added to the FD set. Functional dependency calculation is best-effort, so this can lead to a different resulting FD set, which causes flaky tests. This patch makes the calculation deterministic by iterating instead through a `intsets.Fast` set.

Fixes #107148
Fixes #107162

Release note: None

107181: dev: when cross-building, use `-c opt` r=rail a=rickystewart

This enables optimizations which you probably want for a cross-build.

Epic: CRDB-17171
Release note: None

107183: acceptance: add log dir as a writable path r=rail a=rickystewart

Otherwise you get a sandbox error.

Epic: CRDB-17171
Release note: None

Co-authored-by: Alex Sarkesian <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Sean Barag <[email protected]>
Co-authored-by: Drew Kimball <[email protected]>
blathers-crl bot pushed a commit to cockroachdb/cockroach that referenced this issue Jul 19, 2023
When working with first-party JS dependencies that aren't in this
monorepo, the idiomatic development workflow uses pnpm link [1] to link
multiple JS packages together. Specifically, running
'pnpm link /path/to/github.com/cockroachdb/ui/packages/foo' from within
pkg/ui/workspaces/* creates a symbolic link at
node_modules/@cockroachlabs/foo that points to
../../(…)/ui/packages/foo. This works quite smoothly for local
development, as changes in the 'foo' package are automatically seen by a
'pnpm webpack --watch' running in CRDB. The two packages act like
they're maintained in the same repo, while allowing independent version
history, CI processes, etc.

Unfortunately, rules_js currently offers no way to link outside of the
Bazel workspace. Such a symlink is either ignored by rules_js (since it
doesn't appear in pnpm-lock.yaml) or is rejected if it's manually added
to the lockfile [2]. Allowing Bazel-based builds of CockroachDB when
'pnpm link'ed packages are present introduces dependency skew between
Bazel and non-Bazel builds. To allow 'pnpm link' to be used safely,
pre-emptively reject builds of 'cockroach' and 'cockroach-oss' that are
run through the 'dev' helper when linked @cockroachlabs/ packages are
detected.

[1] https://pnpm.io/cli/link
[2] aspect-build/rules_js#1165

Release note: None
Epic: none
@alexeagle
Copy link
Member

I think it makes sense to allow you to do this.

We've also discussed a bazel wrapper (like Aspect CLI) could give an easier way to apply patches to third-party libraries - imagine you have edits in anything that bazel downloader fetches and could easily go from the repo where you made the edits to a patch file Bazel applies. (issue 828 in our internal monorepo)

@alexeagle alexeagle added help wanted Aspect isn't prioritizing this, but the community could and removed untriaged Requires traige labels Aug 3, 2023
@alexeagle
Copy link
Member

@sjbarag you should be able to use the pnpm.overrides field in the root package.json (next to pnpm-lock.yaml) with a falues like file:/some/path and then after re-running the pnpm install so that path appears in the lockfile, it should work.

I'm not sure if that actually resolves the request, or if the typical pnpm link workflow is actually more convenient than this, and so there's still something we should change in rules_js?

@sjbarag
Copy link
Author

sjbarag commented Aug 4, 2023

@sjbarag you should be able to use the pnpm.overrides field in the root package.json (next to pnpm-lock.yaml) with a falues like file:/some/path and then after re-running the pnpm install so that path appears in the lockfile, it should work.

I'm not sure if that actually resolves the request, or if the typical pnpm link workflow is actually more convenient than this, and so there's still something we should change in rules_js?

Thanks, that gets pretty close! It does risk someone committing that change, but it'd of course fail in CI. Perhaps that's not as big a deal as I'd originally thought.

The main benefits for pnpm link (and anything in the npm link family are:

  1. It doesn't change any source files files, so there's no risk of accidentally committing / submitting to CI a broken package.json
  2. Changes in the host project (the Bazel one, in this case) are picked up automatically without having to rerun pnpm install because of the symlinks involved

Of course, those same symlinks can become a nightmare with Node's resolution algorithm. One can easily end up with multiple copies of react a bundle (react expects to be a singleton), etc. So it's reasonable if y'all don't want to support it for those reasons :)

@jacobgardner
Copy link

jacobgardner commented Oct 11, 2023

@sjbarag you should be able to use the pnpm.overrides field in the root package.json (next to pnpm-lock.yaml) with a falues like file:/some/path and then after re-running the pnpm install so that path appears in the lockfile, it should work.

I'm not sure if that actually resolves the request, or if the typical pnpm link workflow is actually more convenient than this, and so there's still something we should change in rules_js?

Maybe I'm misunderstanding, but I still get the "Invalid link_package outside of the WORKSPACE" error when I try this.

For context, I add the absolute path to the root of the external module in the pnpm overrides:

    "ts-client-generator": "file:/home/jacob/projects/ts-client-generator",

And then when I do pnpm install, it appears to change the absolute path into a relative one with link inside the pnpm-lock.yaml:

      ts-client-generator: link:../../../../projects/ts-client-generator

@alexeagle
Copy link
Member

I think maybe aspect-build/bazel-examples#290 illustrates a solution for this? @gregmagolan is it the same issue?

@gregmagolan
Copy link
Member

gregmagolan commented Oct 11, 2023

One big constraint here is that Bazel can't reference targets outside of the WORKSPACE so you can't just put another repo on disk and pull it into the Bazel graph. That is why an absolute path such as "ts-client-generator": "file:/home/jacob/projects/ts-client-generator" won't work under Bazel since /home/jacob/projects/ts-client-generator is presumably outside of the Bazel WORKSPACE you're building in.

You can bring other repositories on disk into a Bazel graph with local_repository in your WORKSPACE but when you do Bazel will put them under $(bazel info output_base)/external/other_repository_name. That doesn't play nicely with pnpm link since the path on disk where that lives is Bazel managed and the labels to that repository start with @other_repository_name// which can't be expressed in a package.json files or lock file since pnpm deals with paths only. In theory, you could tell rules_js that a certain relative path corresponds to an external repository such as @other_repository_name// but then you still run into issues with referencing npm_package targets in an external repository since the BUILD file there will have references to the @npm repository for that WORKSPACE to link its deps into that package which breaks things. And there is also the problem of transitive deps of that npm_package not being in the virtual store of the workspace you're building in.

Long story short, it is tricky to reproduce the pnpm link workflow under Bazel since external deps have to be brought in via the WORKSPACE with Bazel. aspect-build/bazel-examples#290 is a POC of bringing in npm_packages from other repositories, although the use case is geared towards a permanent reference to the npm_package from the other repo vs. a temporary change for local development.

@gregmagolan
Copy link
Member

There may be a pattern where you pnpm link a .gitignored but not .bazelignored within your WORKSPACE.

In this use case, is the dep being pnpm linked one that is built with Bazel and has a npm_package target for rules_js to reference?

twheys added a commit to twheys/rules_js that referenced this issue Oct 24, 2023
…col for package.json versions that point to packages external to the workspace
@benmcginnis
Copy link

benmcginnis commented Jun 27, 2024

In our case we are developing an open source library without bazel, but want to be able to test integrating it into our internal bazel powered monorepo without publishing a release of the library to npm, and so being able to have the file linkages would be really nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Aspect isn't prioritizing this, but the community could
Projects
Status: No status
Development

No branches or pull requests

6 participants