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

feat: download the latest version instead of a pinned one #134

Merged
merged 7 commits into from
Aug 19, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
13 changes: 11 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,16 @@ Just use your package managers as you usually would. Run `yarn install` in Yarn

## Known Good Releases

When running Yarn or pnpm within projects that don't list a supported package manager, Corepack will default to a set of Known Good Releases. In a way, you can compare this to Node.js, where each version ships with a specific version of npm.
When running Corepack within projects that don't list a supported package
manager, it will default to a set of Known Good Releases. In a way, you can
compare this to Node.js, where each version ships with a specific version of npm.

The Known Good Releases can be updated system-wide using the `--activate` flag from the `corepack prepare` and `corepack hydrate` commands.
If there is no Known Good Release for the requested package manager, Corepack
Copy link
Member

Choose a reason for hiding this comment

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

In what case would there be no Known Good Release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upon the first time Corepack is run with a given package manager. The idea behind this PR would be not to rely on what version is set on config.json but instead get the most up-to-date version from npm registry.

Copy link
Member

@styfle styfle Jul 9, 2022

Choose a reason for hiding this comment

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

How often does it check the registry? Does it check every time you invoke yarn/pnpm/npm because that seems slow. Especially if I just want to do npm run <script> which should work offline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, only the first time, at which point it saves it as Known Good Release. Users can then update (or downgrade) that version by using corepack prepare or corepack hydrate (e.g. corepack prepare pnpm@latest --activate to get the latest pnpm by default). We could make an automated process for the Known Good Version to expire after some time, but that's not part of this initial implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I understand now 👍

I think expiring would could be confusing. I also think this any auto update could be confusing because it no longer ties the package manager version to the corepack version (or node version for that matter).

I think it’s best to let the user decide when to upgrade and perhaps provide better docs how to upgrade (this isn’t super clear today). Without corepack, this is handled with a notification that says something like npm i -g npm@latest.

Has any of this behavior been reviewed by the npm team? I think they had some ideas about Know Good Release.

Copy link
Contributor Author

@aduh95 aduh95 Jul 9, 2022

Choose a reason for hiding this comment

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

I also think this any auto update could be confusing because it no longer ties the package manager version to the corepack version (or node version for that matter).

Well that's the reason Corepack exists, because the Node.js TSC doesn't want to have specific version of a package manager tied in with a specific Node.js version – specifically, several Node.js TSC members have expressed that the reason they are supportive of Corepack is to get rid of having to do security releases every time a vulnerability is reported in npm rather than node itself.
So the auto-lookup during the first use has to be the default behavior.

I think it’s best to let the user decide when to upgrade and perhaps provide better docs how to upgrade (this isn’t super clear today). Without corepack, this is handled with a notification that says something like npm i -g npm@latest.

Yes that makes sense, letting the end-user in control is very much the goal here. So instead of npm i -g npm@latest, users would type corepack prepare npm@latest --activate, which is arguably quite similar. Improving docs is of course always a goal.

Has any of this behavior been reviewed by the npm team? I think they had some ideas about Know Good Release.

The npm team hasn't shown much interest in the development of Corepack, so I don't think they have.

looks up the npm registry for the latest available version and cache it for
future use.

The Known Good Releases can be updated system-wide using the `--activate` flag
from the `corepack prepare` and `corepack hydrate` commands.

## Offline Workflow

Expand Down Expand Up @@ -106,6 +113,8 @@ This command will retrieve the given package manager from the specified archive

- `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_NO_LOOKUP` can be set in order to instruct Corepack not to lookup on the remote registry for the latest version of the selected package manager.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably find a better name for this env variable, I don't think one could infer what it's doing from the name only. Does anyone has suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to understand the difference between COREPACK_ENABLE_NETWORK=0 and COREPACK_NO_LOOKUP=1? In theory I would expect no network to behave the same since corepack ships with Known Good Versions but I must be overlooking a use case here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When COREPACK_NO_LOOKUP is set, Corepack will use the version defined in config.json instead of asking the npm registry for the most up-to-date version. COREPACK_ENABLE_NETWORK can disable all access to the network, both for finding what's the most up-to-date version number and to download the tarball.

Copy link
Member

Choose a reason for hiding this comment

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

Is config.json available to configure by the user of corepack or is it just internals?

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's only internals, bundled in the corepack executable.

Copy link
Member

Choose a reason for hiding this comment

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

How about COREPACK_IGNORE_LATEST?

Copy link
Contributor Author

@aduh95 aduh95 Jul 23, 2022

Choose a reason for hiding this comment

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

I went with COREPACK_DEFAULT_TO_LATEST which is true by default, and is set to 0 to use the value in config.json instead.

Copy link
Member

@styfle styfle Jul 24, 2022

Choose a reason for hiding this comment

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

Not sure about using “default” as a verb here.

How about COREPACK_ENABLE_FETCH_LATEST=0

Copy link
Contributor Author

@aduh95 aduh95 Jul 24, 2022

Choose a reason for hiding this comment

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

I'd like the naming to make clear that it's only relevant if there's no local clue on what version to use. ENABLE_FETCH_LATEST could mean that the latest is always downloaded, or at least that's how I read it.

Not sure about using “default” as a verb here.

I'm not a native english speaker, so genuine question: is it incorrect or something to use "default" as a verb?

Copy link
Member

Choose a reason for hiding this comment

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

It’s not incorrect, just that “default” can be a verb or a noun. It’s probably fine 👍


- `COREPACK_HOME` can be set in order to define where Corepack should install the package managers. By default it is set to `$HOME/.node/corepack`.

- `COREPACK_ROOT` has no functional impact on Corepack itself; it's automatically being set in your environment by Corepack when it shells out to the underlying package managers, so that they can feature-detect its presence (useful for commands like `yarn init`).
Expand Down
12 changes: 12 additions & 0 deletions config.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
"definitions": {
"npm": {
"default": "8.13.1",
"fetchLatestFrom": {
"type": "npm",
"package": "npm"
},
"transparent": {
"commands": [
[
Expand Down Expand Up @@ -29,6 +33,10 @@
},
"pnpm": {
"default": "7.3.0",
"fetchLatestFrom": {
"type": "npm",
"package": "pnpm"
},
"transparent": {
"commands": [
[
Expand Down Expand Up @@ -71,6 +79,10 @@
},
"yarn": {
"default": "1.22.19",
"fetchLatestFrom": {
"type": "npm",
"package": "yarn"
},
"transparent": {
"default": "3.2.1",
"commands": [
Expand Down
22 changes: 15 additions & 7 deletions sources/Engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,17 +69,25 @@ export class Engine {
// Ignore errors; too bad
}

if (typeof lastKnownGood !== `object` || lastKnownGood === null)
return definition.default;
if (typeof lastKnownGood === `object` && lastKnownGood !== null &&
Object.prototype.hasOwnProperty.call(lastKnownGood, packageManager)) {
const override = (lastKnownGood as any)[packageManager];
if (typeof override === `string`) {
return override;
}
}

if (!Object.prototype.hasOwnProperty.call(lastKnownGood, packageManager))
if (process.env.COREPACK_NO_LOOKUP)
return definition.default;

const override = (lastKnownGood as any)[packageManager];
if (typeof override !== `string`)
return definition.default;
const reference = await corepackUtils.fetchLatestStableVersion(definition.fetchLatestFrom);

await this.activatePackageManager({
name: packageManager,
reference,
});

return override;
return reference;
}

async activatePackageManager(locator: Locator) {
Expand Down
17 changes: 17 additions & 0 deletions sources/corepackUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,23 @@ import * as httpUtils from './httpUtils
import * as nodeUtils from './nodeUtils';
import {RegistrySpec, Descriptor, Locator, PackageManagerSpec} from './types';

export async function fetchLatestStableVersion(spec: RegistrySpec) {
switch (spec.type) {
case `npm`: {
const {[`dist-tags`]: {latest}, versions: {[latest]: {dist: {shasum}}}} =
Copy link
Member

@styfle styfle Jul 24, 2022

Choose a reason for hiding this comment

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

Should this consider the major version? What about when npm 7 bumped to npm 8 to drop support for node 12. Imagine if corepack existed then and suddenly npm stopped working because of the automatic fetch latest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That’s worth discussing, but imo it’s ok to ignore that for the initial implementation.

await httpUtils.fetchAsJson(`https://registry.npmjs.org/${spec.package}`);
return `${latest}+sha1.${shasum}`;
}
case `url`: {
const data = await httpUtils.fetchAsJson(spec.url);
return data[spec.fields.tags].stable;
}
default: {
throw new Error(`Unsupported specification ${JSON.stringify(spec)}`);
}
}
}

export async function fetchAvailableTags(spec: RegistrySpec): Promise<Record<string, string>> {
switch (spec.type) {
case `npm`: {
Expand Down
5 changes: 5 additions & 0 deletions sources/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ export interface Config {
*/
default: string;

/**
* Defines how to fetch the latest version from a remote registry.
*/
fetchLatestFrom: RegistrySpec;

/**
* Defines a set of commands that are fine to run even if the user isn't
* in a project configured for the specified package manager. For instance,
Expand Down
1 change: 1 addition & 0 deletions tests/main.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {runCli} from './_runCli';

beforeEach(async () => {
process.env.COREPACK_HOME = npath.fromPortablePath(await xfs.mktempPromise());
process.env.COREPACK_NO_LOOKUP = `1`;
});

const testedPackageManagers: Array<[string, string]> = [
Expand Down