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

feat: add COREPACK_ENABLE_STRICT env variable #167

Merged
merged 1 commit into from
Sep 1, 2022
Merged

feat: add COREPACK_ENABLE_STRICT env variable #167

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
10 changes: 8 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,18 @@ This command will retrieve the given package manager from the specified archive

## Environment Variables

- `COREPACK_ENABLE_NETWORK` can be set to `0` to prevent Corepack from accessing the network (in which case you'll be responsible for hydrating the package manager versions that will be required for the projects you'll run, using `corepack hydrate`).

- `COREPACK_DEFAULT_TO_LATEST` can be set to `0` in order to instruct Corepack
not to lookup on the remote registry for the latest version of the selected
package manager.

- `COREPACK_ENABLE_NETWORK` can be set to `0` to prevent Corepack from accessing
the network (in which case you'll be responsible for hydrating the package
manager versions that will be required for the projects you'll run, using
`corepack hydrate`).

- `COREPACK_ENABLE_STRICT` can be set to `0` to prevent Corepack from checking
Copy link

Choose a reason for hiding this comment

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

It's a bit weird that the name is COREPACK_ENABLE_STRICT but the default is true. You only use this environment variable when you want to disable strict checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It follows the same pattern as COREPACK_ENABLE_NETWORK. Do you have another name suggestion?

Copy link
Member

@styfle styfle Aug 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I prefer env vars and booleans alike to always be named "enable" regardless of default value so you don't have to think about double negatives when disabling the "disable" variable.

This allows you to change the default value in the future without renaming the env var which is really nice if the consumer needs to set the env var and doesn't know the version of corepack that will be used.

TLDR: keep it as _ENABLE_

Copy link
Member

Choose a reason for hiding this comment

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

Separately tho, it's always weird and confusing when a boolean option has different behavior between "absent" and "false".

Copy link

Choose a reason for hiding this comment

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

I don't have another naming suggestion given that this follows an existing pattern. I'm not trying to block or delay this with my comment.

if the package manager corresponds to the one defined for the current project.

- `COREPACK_HOME` can be set in order to define where Corepack should install
the package managers. By default it is set to `%LOCALAPPDATA%\node\corepack`
on Windows, and to `$HOME/.cache/node/corepack` everywhere else.
Expand Down
2 changes: 2 additions & 0 deletions sources/specUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ export async function findProjectSpec(initialCwd: string, locator: Locator, {tra
// A locator is a valid descriptor (but not the other way around)
const fallbackLocator = {name: locator.name, range: locator.reference};

if (process.env.COREPACK_ENABLE_STRICT === `0`) return fallbackLocator;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: might not be needed in this PR, but it'd make sense to move the "parsing" logic in a function so we could be sure the same one is used for enableStrict and enableNetwork


while (true) {
const result = await loadSpec(initialCwd);

Expand Down
28 changes: 28 additions & 0 deletions tests/main.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,34 @@ it(`should allow to call "prepare" without arguments within a configured project
});
});

it(`should refuse to run a different package manager within a configured project`, async () => {
await xfs.mktempPromise(async cwd => {
await xfs.writeJsonPromise(ppath.join(cwd, `package.json` as Filename), {
packageManager: `[email protected]`,
});

process.env.FORCE_COLOR = `0`;

await expect(runCli(cwd, [`pnpm`, `--version`])).resolves.toMatchObject({
stdout: `Usage Error: This project is configured to use yarn\n\n$ pnpm ...\n`,
exitCode: 1,
});

// Disable strict checking to workaround the UsageError.
process.env.COREPACK_ENABLE_STRICT = `0`;

try {
await expect(runCli(cwd, [`pnpm`, `--version`])).resolves.toMatchObject({
stdout: `${config.definitions.pnpm.default.split(`+`, 1)[0]}\n`,
exitCode: 0,
});
} finally {
delete process.env.COREPACK_ENABLE_STRICT;
delete process.env.FORCE_COLOR;
}
});
});

it(`should allow to call "prepare" with --all to prepare all package managers`, async () => {
await xfs.mktempPromise(async cwd => {
await xfs.writeJsonPromise(ppath.join(cwd, `package.json` as Filename), {
Expand Down