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

Don't install dependencies in PnP projects #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arendjr
Copy link

@arendjr arendjr commented Oct 23, 2020

Note I moved the installation of dependencies to a separate function. This has the side-effect of detecting whether PnP/Yarn/package-lock are used twice. I think this is desired, as it'll make the script work correctly even across revisions where a project decided to switch their package management solution.

async function installDependencies(revision) {
const cwd = process.cwd();
if (await fileExists(path.resolve(cwd, '.pnp.js'))) {
return; // No need to install dependencies in projects with PnP enabled.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct, it should run an install either way as the .yarn/unplugged folder is usually not commited.

You can detect Yarn 2 by checking the yarn.lock file for __metadata: and running yarn --immutable to install

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, interesting. I guess we can indeed include #41 into this PR to offer generic Yarn 2 support. But the Zero Install documentation suggests people may also check-in the unplugged directories (indeed, we do for our project). Now I could simply detect whether the unplugged directory exists, and if it does we still don't need to install. But Git doesn't maintain empty directories... so if it doesn't exist, does that mean we should install (at least it's the safe thing to do) or doesn't it exist because no packages had postinstall scripts? Would it be too convoluted to check .gitignore, and assume we need to do the install if we see unplugged is listed there?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The easiest and safest way forward is to always trigger the install, if you're using Zero-Installs and commit everything it's basically a noop.

I guess we can indeed include #41 into this PR to offer generic Yarn 2 support

The existance, or lack thereof, of a .yarnrc.yml file isn't enough, the correct way to check is to grep yarn.lock for __metadata:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a reference to somewhere the __metadata is documented? I do believe you, but it feels like relying on an incidental side-effect of the lock file to detect the version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did find this in the docs: https://yarnpkg.com/getting-started/migration

The main change is the name of the file. Yarn 1 used .yarnrc, but Yarn 2 is moving to a different name: .yarnrc.yml. This should make it easier for third-party tools to detect whether a project uses Yarn 1 or Yarn 2, and will allow you to easily set different settings in your home folders when working with a mix of Yarn 1 and Yarn 2 projects.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a reference to somewhere the __metadata is documented?

I don't, but as one of the maintainers of Yarn I can suggest using this to detect it.

I did find this in the docs: https://yarnpkg.com/getting-started/migration

That was true at the time it was written, but once we get https://github.com/nodejs/corepack released with node that file is not guaranteed to exist (it isn't either way but oh well).

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.

None yet

2 participants