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

Yarn PnP Support #1599

Closed
Toxicable opened this issue Feb 3, 2020 · 15 comments · Fixed by #3195
Closed

Yarn PnP Support #1599

Toxicable opened this issue Feb 3, 2020 · 15 comments · Fixed by #3195

Comments

@Toxicable
Copy link

Yarn 2.0 has been released. https://dev.to/arcanis/introducing-yarn-2-4eh1
This release has some massive changes, the biggest one is using PnP by default. https://next.yarnpkg.com/features/pnp
However, we can disable the use of this with a pluin for a first cut, then later on aim to support PnP.

The migration guide is here https://next.yarnpkg.com/advanced/migration
From what I can see, with keeping node_modules, there aren't too many big changes, needs more investigation.

@pauldraper
Copy link

PnP is pretty neat, as it allows strong flexibility in linking, giving better performance.

I actually thing it could be appealing to use in rule_nodejs for everything.

@payload
Copy link

payload commented Feb 23, 2020

How would this be done? As far as I understand .pnp.js resolves paths. Would this script be queried to generate build files?

@ashi009
Copy link
Contributor

ashi009 commented Aug 25, 2020

.pnp.js's approach to the package resolution is quite similar to the rules_nodejs's, or to say, the right way to handle package resolution.

Apart from the pnp, a few things that I want from yarn 2 are:

  • portal: support, to allow absolute package lookup in the workspace without any post-install magic
  • patch: support, to replace patch-package
  • install @types automatically

I guess adopting yarn2 is not easy, as all the existing rules are built around the node_modules assumption and many need to be rewritten to take advantage of the pnp. On the other hand, support yarn 2 and drop the support of npm and yarn 1.x in the next major release will significantly simplify some rules implementation, eg. nodejs_binary and ts_library, given the efforts that facebook are put into to make pnp work on all major libraries, including typescript.

@Toxicable
Copy link
Author

@ashi009 I've done a small prototype here #1856 and you're right it makes the implementation of the rules in this repository simple.
However there are still a few issues im not certain about.

  • Are we able to replace replicate all nodejs_binary's with yarn2_nodejs_binary?, are there edge cases I might have missed?
  • How does a global cache work with remote execution? does it force you to use "Zero Installs"?
  • What is the general community support for yarn2? Would a user face an issue with yarn2 that they wouldn't with pure node?
  • How would it work with the rest of this repository? There's no chance of only supporting yarn2 alone so we'd need to support 2 methods of executing a nodejs program

I havn't had much time to work on it lately so im open for any contributions here

@ashi009
Copy link
Contributor

ashi009 commented Aug 25, 2020

@Toxicable I have answers to some of those (I guess):

  • How does a global cache work with remote execution? does it force you to use "Zero Installs"?

Probably like how rules_go handles this. Install dependencies in a repository rule declared directory and cache those files. But I guess it's not particularly remote execution friendly.

  • What is the general community support for yarn2? Would a user face an issue with yarn2 that they wouldn't with pure node?

Let facebook deal with that, they are staffed to roll yarn2 forward.

  • How would it work with the rest of this repository? There's no chance of only supporting yarn2 alone so we'd need to support 2 methods of executing a nodejs program

I don't think supporting both sound. After all, people will only keep either package.lock, yarn.lock or the new yaml yarn.lock in a single workspace. If yarn2 was the chosen one (I hope), there should be a breaking change in rules_nodejs with some automated tool for the migration.

@merceyz
Copy link

merceyz commented Aug 25, 2020

Just to note: you can use Yarn v2 with node_modules as a stepping stone, just run yarn config set nodeLinker node-modules.

Let facebook deal with that, they are staffed to roll yarn2 forward.

Facebook has nothing to do with Yarn https://yarnpkg.com/advanced/qa/#is-yarn-operated-by-facebook

@lummax
Copy link

lummax commented Apr 9, 2021

As a first step, it would be nice to enable yarn2 support when running in a yarn compatible mode. I was successful with

node_repositories(
    node_version = "14.8.0",
    vendored_yarn = "@wksp//projects/support/bazel/toolchains/javascript:yarn",
    yarn_version = "2.4.1",
)

yarn_install(
    name="test"
    args = ["--immutable"] 
    frozen_lockfile = False,  # make work with yarn2
    use_global_yarn_cache = False,  # make work with yarn2
    data = [":.yarnrc.yml"],
    package_json = ":package.json",
    yarn_lock = ":yarn.lock",
)

With the following .yarnrc.yml

nodeLinker: node-modules
yarnPath: <...input correct path...>/bin/yarn.js

For that to work I had to set the option as shown above and delete https://github.com/bazelbuild/rules_nodejs/blob/aa09b5796faaf2470b84246a8f00e6e27181c63b/internal/npm_install/npm_install.bzl#L431. So for minimal support I see three options:

  1. add a flag to remove the automatic addition of --mutex network
  2. add a bool attribute yarn2_compatible that does not use --mutex network and replaces --frozen-lockfile with --immutable
  3. detect that yarn2 is used and behave accordingly

I'd be happy to provide a PR, but am unsure which of these three options is preferred.

Note that this is not yet real support as PNP will not work / a special .yarnrc.yml and a vendored yarn2 is needed. But it would be a first step.

@emilio-martinez
Copy link

To second @lummax, I think it would be awesome if Bazel could support yarn berry, which I'd consider as the only actively maintained version at the time of this writing. Working at least with the node_modules nodeLinker, while not yarn berry's default, I think would definitely be a good first step, if only because it's has the most "compatible" support within the ecosystem. Without that minimum baseline, using Bazel is kind of a no-go for projects that would like to explore adopting it.

Are there any good next steps? Should the direction be to make modifications mentioned above, or should this be it's own yarn_berry_install?

@lodmfjord
Copy link

Any news on this?

Using yarn berry currently. It is even speedier with pnp.

@alexeagle
Copy link
Collaborator

There's a recent proposal to make pnpm semantics in npm itself. Given limited time of maintainers here I think we have to focus exclusively on the "canonical" tooling which is npm. If anyone wants to maintain yarn-for-Bazel it would be interesting to discuss forking that feature set to a separate repo.

@alexeagle
Copy link
Collaborator

npm/rfcs#436
Is the proposal that I think we should align with

@alexeagle alexeagle added the Can Close? We will close this in 30 days if there is no further activity label Aug 30, 2021
@github-actions github-actions bot removed the Can Close? We will close this in 30 days if there is no further activity label Aug 31, 2021
@arcanis
Copy link

arcanis commented Sep 3, 2021

Fwiw this issue should rather be called "PnP support", since Yarn 2+ can install classical node_modules trees as well - it's just quite slower and suffers from the same issues as it always did (heavier, atomic issues, very complex to implement and maintain, imperfect peer dependencies, etc). Also note that PnP is supported by both Yarn & pnpm, so this is larger than just one package manager.

As for npm/rfcs#436, imo it should be another issue about broader "Symlink-based node_modules support" in general (which I think has value, Yarn as well will support it as another install strategy). Note however that, just like classical node_modules, it has its own set of problems (for instance, some peer dependencies patterns simply cannot be represented when using workspaces). PnP is the only strategy that has the ability to satisfy all semantic requirements (making it quite valuable on its own).

the "canonical" tooling which is npm

npm isn't canonical by any means - the ecosystem seem fairly evenly split between package managers. In fact, even future Node release will tend to provide more options than just npm through the Corepack initiative.

@alexeagle
Copy link
Collaborator

Hey @arcanis thanks for stopping by! I agree PnP support is the real request here. We are kinda novice at the details of what that means for the package manager itself. We probably have a conflated view of silos where you see the nuance of the layering.

We are starting work on this soon. We start from tarballs downloaded from the registry, and have control needed to optimize how these end up on disk for each Bazel action. It's like we need to install every time we run node, so it has to be fast. Maybe we'll ping you for help!

@alexeagle
Copy link
Collaborator

#3071 tracked supporting yarn 2+, let's continue using this one for supporting PnP. The former is solved at HEAD but the latter isn't, so I reopened this.

@alexeagle
Copy link
Collaborator

This repo no longer supports any package managers. aspect-build/rules_js supports pnpm, and others are free to make rulesets specific to Yarn or npm (or whatever new flavor-of-the-month is invented next)

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 a pull request may close this issue.