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

Jump to bottom

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

Merged
merged 7 commits into from
Aug 19, 2022
Merged

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

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
88f6807
feat: download the latest version instead of a pinned one
aduh95 Jun 24, 2022
6c9e041
add `COREPACK_NO_LOOKUP` env flag
aduh95 Jun 26, 2022
0668fb0
Use KGR to cache the version
aduh95 Jun 27, 2022
c33fa1d
don't hardcode the registry
aduh95 Jun 28, 2022
12a91f9
Merge branch 'main'
aduh95 Jul 23, 2022
4ce5e2c
`COREPACK_NO_LOOKUP` -> `COREPACK_DEFAULT_TO_LATEST`
aduh95 Jul 23, 2022
9563bcd
Merge branch 'main' into dynamic-default-version
aduh95 Aug 19, 2022
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
15 changes: 13 additions & 2 deletions README.md
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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,10 @@ 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_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_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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
"definitions": {
"npm": {
"default": "8.15.0+sha1.d4b53cd29b13ea164f0f5767bca274dbe7d8f78d",
"fetchLatestFrom": {
"type": "npm",
"package": "npm"
},
"transparent": {
"commands": [
[
Expand Down Expand Up @@ -29,6 +33,10 @@
},
"pnpm": {
"default": "7.5.2+sha1.89331cc7dc542e0d7a33fe4f423989f0c54e9604",
"fetchLatestFrom": {
"type": "npm",
"package": "pnpm"
},
"transparent": {
"commands": [
[
Expand Down Expand Up @@ -71,6 +79,10 @@
},
"yarn": {
"default": "1.22.19+sha1.4ba7fc5c6e704fce2066ecbfb0b0d8976fe62447",
"fetchLatestFrom": {
"type": "npm",
"package": "yarn"
},
"transparent": {
"default": "3.2.2+sha224.634d0331703700cabfa9d9389835bd8f7426b0207ed6b74d8d34c81e",
"commands": [
Expand Down
23 changes: 16 additions & 7 deletions sources/Engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {UsageError} from 'clipanion';
import fs from 'fs';
import path from 'path';
import process from 'process';
import semver from 'semver';

import defaultConfig from '../config.json';
Expand Down Expand Up @@ -69,17 +70,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_DEFAULT_TO_LATEST === `0`)
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,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 •
edited
Loading

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
View file
Open in desktop
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
View file
Open in desktop
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_DEFAULT_TO_LATEST = `0`;
});

it(`should refuse to download a package manager if the hash doesn't match`, async () => {
Expand Down