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

Add support for Yarn Plug'n'Play #158

Open
benblank opened this issue Sep 28, 2018 · 8 comments
Open

Add support for Yarn Plug'n'Play #158

benblank opened this issue Sep 28, 2018 · 8 comments

Comments

@benblank
Copy link
Contributor

benblank commented Sep 28, 2018

NOTE: this issue predates this project's rename to Volta.

Notion currently only supports traditional node_modules dependencies. Support should be added for Yarn Plug'n'Play once it stabilizes.

@arcanis
Copy link

arcanis commented Feb 6, 2019

Why is Notion using node_modules at all? Aren't the tools installed globally?

@benblank
Copy link
Contributor Author

benblank commented Feb 6, 2019

Notion supports (will support?) delegating from a globally-installed tool to a project-specific version if one exists. (For example, if you have tsc installed globally but the project you're in uses a different version, the project's version will be used inside that project.) Because of this, Notion needs some knowledge of where those project-specific binaries are located.

@arcanis
Copy link

arcanis commented Feb 7, 2019

I see - the best way I see this being solved would be to defer the project packages execution to Yarn itself (which is what we recommend anyway - the symlinks being in node_modules/.bin are an implementation detail, and as such directly calling them is an undefined behavior).

So instead of:

$> node_modules/.bin/eslint --foo --bar

You'd actually run:

$> yarn run eslint --foo --bar
// or
$> yarn node "$(yarn bin eslint)" --foo --bar

Does that make sense?

@dherman
Copy link
Collaborator

dherman commented Feb 8, 2019

@arcanis Is there a way we can reliably cache the information you're getting from yarn bin etc, so that we're both (a) not relying on implementation details but (b) not having to take the latency hit of booting up Yarn every time we run a command? (Not a criticism of Yarn, btw! It's just a goal of Notion to avoid the overhead of things like firing up a Node VM.)

@arcanis
Copy link

arcanis commented Feb 8, 2019

You can expect the value of yarn bin eslint to change if:

  • The user moves from a project to another (because the cache path might be different from one to another), so it would go from /project-1/cache/eslint-1.0.0/bin/eslint to /project-2/....
  • The Yarn dependency tree changes in some way (usually because eslint is updated), which would make it go from /project/cache/eslint-1.0.0/bin/eslint to /.../eslint-2.0.0/...

The Node boot time is unfortunate indeed :(

@charlespierce
Copy link
Contributor

@dherman @arcanis Circling back to this after a lot has changed, I think we can implement exactly the solution of running yarn run <tool> <args> without too much difficulty.

We now know that a project has local bins by looking at the dependencies and devDependencies to make sure that project has the appropriate package as a dependency, then calling node_modules/.bin/<tool> <args>. We're already reading package.json to detect the dependencies, so we can also read the installConfig setting to determine if Yarn PnP is enabled and switch the command we call based on that.

See https://github.com/volta-cli/volta/blob/master/crates/volta-core/src/run/binary.rs#L26 We can add a check here if project.is_yarn_pnp and from that, either call the binary directly or call yarn run.

@charlespierce
Copy link
Contributor

Since PnP is now on by default in Yarn 2.0 (and from what I can tell with the docs, doesn't require the installConfig setting), I think the most robust way to detect PnP would be to look for the .pnp.js file that Yarn generates. If that is there (and the platform includes Yarn), then we can delegate to `yarn run .

@nemonemi
Copy link

nemonemi commented Sep 3, 2021

Just a note: https://github.com/yarnpkg/berry/blob/master/CHANGELOG.md#300

Yarn will now generate .pnp.cjs files (instead of .pnp.js) when using PnP

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants