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.

Sign up for GitHub

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
Open

Don't install dependencies in PnP projects #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion index.js
View file
Open in desktop

Large diffs are not rendered by default.

47 changes: 27 additions & 20 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,26 +27,11 @@ async function run(octokit, context, token) {

console.log(`PR #${pull_number} is targetted at ${pr.base.ref} (${pr.base.sha})`);

await installDependencies(`current`);

const buildScript = getInput('build-script') || 'build';
const cwd = process.cwd();

const yarnLock = await fileExists(path.resolve(cwd, 'yarn.lock'));
const packageLock = await fileExists(path.resolve(cwd, 'package-lock.json'));

let npm = `npm`;
let installScript = `npm install`;
if (yarnLock) {
installScript = npm = `yarn --frozen-lockfile`;
}
else if (packageLock) {
installScript = `npm ci`;
}

startGroup(`[current] Install Dependencies`);
console.log(`Installing using ${installScript}`)
await exec(installScript);
endGroup();

startGroup(`[current] Build using ${npm}`);
console.log(`Building using ${npm} run ${buildScript}`);
await exec(`${npm} run ${buildScript}`);
Expand Down Expand Up @@ -89,9 +74,7 @@ async function run(octokit, context, token) {
}
endGroup();

startGroup(`[base] Install Dependencies`);
await exec(installScript);
endGroup();
await installDependencies(`base`);

startGroup(`[base] Build using ${npm}`);
await exec(`${npm} run ${buildScript}`);
Expand Down Expand Up @@ -234,6 +217,30 @@ async function createCheck(octokit, context) {
};
}

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).

}

const yarnLock = await fileExists(path.resolve(cwd, 'yarn.lock'));
const packageLock = await fileExists(path.resolve(cwd, 'package-lock.json'));

let npm = `npm`;
let installScript = `npm install`;
if (yarnLock) {
installScript = npm = `yarn --frozen-lockfile`;
}
else if (packageLock) {
installScript = `npm ci`;
}

startGroup(`[${revision}] Install Dependencies`);
console.log(`Installing using ${installScript}`)
await exec(installScript);
endGroup();
}

(async () => {
try {
const token = getInput('repo-token', { required: true });
Expand Down