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: match npm semantics exactly for installs and updates #296

Merged
merged 11 commits into from
Apr 29, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
4 changes: 2 additions & 2 deletions chompfile.toml
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,10 @@ run = '''
[[task]]
name = 'prettier'
template = 'prettier'
deps = ['src/**/*.ts', 'test/**/*.ts']
deps = ['src/**/*.ts', 'test/**/*.js']
[task.template-options]
ignore-path = '.prettierignore'
files = 'src/**/*.ts test/**/*.ts'
files = 'src/**/*.ts test/**/*.js'
loglevel = 'warn'

[[task]]
Expand Down
101 changes: 63 additions & 38 deletions src/generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import { getIntegrity } from "./common/integrity.js";
import { createLogger, Log, LogStream } from "./common/log.js";
import { Replacer } from "./common/str.js";
import { analyzeHtml } from "./html/analyze.js";
import { InstallTarget } from "./install/installer.js";
import { InstallTarget, type InstallMode } from "./install/installer.js";
import { LockResolutions } from "./install/lock.js";
import { getDefaultProviderStrings, type Provider } from "./providers/index.js";
import * as nodemodules from "./providers/nodemodules.js";
Expand Down Expand Up @@ -300,16 +300,17 @@ export interface GeneratorOptions {
/**
* When using a lockfile, do not modify any existing resolutions, and use
* existing resolutions whenever possible for new locks.
*
* @deprecated Use install/link/update to manage dependencies.
*/
freeze?: boolean; // TODO: deprecate and move to install/link options
freeze?: boolean;

/**
* When using a lockfile, force update touched resolutions to latest.
* When using a lockfile, force update all touched resolutions to latest.
*
* @deprecated Defaults to 'true' for installs and updates, set to 'false'
* to enable old behaviour.
* @deprecated
*/
latest?: boolean; // TODO: deprecate and move to install/link options
latest?: boolean;

/**
* Support tracing CommonJS dependencies locally. This is necessary if you
Expand Down Expand Up @@ -363,8 +364,9 @@ export class Generator {
*/
installCnt = 0;

// TODO: remove these and make them options on install/link etc instead.
// @deprecated
private freeze: boolean | null;
// @deprecated
private latest: boolean | null;

/**
Expand Down Expand Up @@ -517,7 +519,7 @@ export class Generator {
this.map = new ImportMap({ mapUrl: this.mapUrl, rootUrl: this.rootUrl });
if (inputMap) this.addMappings(inputMap);

// Set global installation options:
// Set deprecated global resolution options for backwards compat:
this.latest = latest;
this.freeze = freeze;
}
Expand All @@ -530,7 +532,6 @@ export class Generator {
* @param mapUrl An optional URL for the map to handle relative resolutions, defaults to generator mapUrl.
* @param rootUrl An optional root URL for the map to handle root resolutions, defaults to generator rootUrl.
* @returns The list of modules pinned by this import map or HTML.
*
*/
async addMappings(
jsonOrHtml: string | IImportMap,
Expand Down Expand Up @@ -621,11 +622,7 @@ export class Generator {
this.traceMap.visit(
specifier,
{
installOpts: {
freeze: this.freeze ?? true, // link defaults to freeze
latest: this.latest,
mode: "new-prefer-existing",
},
installMode: "freeze",
toplevel: true,
},
parentUrl || this.baseUrl.href
Expand Down Expand Up @@ -1014,15 +1011,26 @@ export class Generator {
async install(
install?: string | Install | (string | Install)[]
): Promise<void | { staticDeps: string[]; dynamicDeps: string[] }> {
if (arguments.length > 1)
throw new JspmError(
"Install takes no arguments, a single install target, or a list of install targets."
);
return this._install(install);
}

private async _install(
Comment on lines 1011 to +1017
Copy link
Member

Choose a reason for hiding this comment

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

Not sure I fully understand the reason for this private _install function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on whether install is given an argument or not, two different modes are used:

  1. No argument - all top-level locks installed in default mode, i.e. keep all in-range locks.
  2. Arguments - all arguments installed in latest-primaries mode, i.e. bump those arguments to latest but keep in-range secondaries.

This just lets us handle those cases without exposing the mode argument in the public API.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I missed that we were keeping mode part of the private API. Should we perhaps just expose it though with defaults? Is there a risk in doing that?

install?: string | Install | (string | Install)[],
mode?: InstallMode
): Promise<void | { staticDeps: string[]; dynamicDeps: string[] }> {
// Backwards-compatibility for deprecated options:
if (this.latest) mode ??= "latest-primaries";
if (this.freeze) mode ??= "freeze";

// If there are no arguments, then we reinstall all the top-level locks:
if (!install) {
if (install === null || install === undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

This was previously handled via generator.reinstall. I believe this change means that reinstall is now deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reinstall() method is equivalent to freeze installing all the top-level locks (or equivalently doing an argumentless link, since link uses freeze by default). So I think it can be deprecated now in favour of either of those, yeah.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's do that then, either with this PR or via a follow-up after it lands.

await this.traceMap.processInputMap;
return this.install(

// To match the behaviour of an argumentless `npm install`, we use
// existing resolutions for everything unless it's out-of-range:
mode ??= "default";

return this._install(
Object.entries(this.traceMap.installer.installs.primary).map(
([alias, target]) => {
const pkgTarget =
Expand All @@ -1046,7 +1054,8 @@ export class Generator {
subpath: target.installSubpath ?? undefined,
} as Install;
}
)
),
mode
);
}

Expand All @@ -1060,7 +1069,7 @@ export class Generator {
}

return await Promise.all(
install.map((install) => this.install(install))
install.map((install) => this._install(install, mode))
).then((installs) => installs.find((i) => i));
}

Expand All @@ -1077,11 +1086,14 @@ export class Generator {
});
return await Promise.all(
install.subpaths.map((subpath) =>
this.install({
target: (install as Install).target,
alias: (install as Install).alias,
subpath,
})
this._install(
{
target: (install as Install).target,
alias: (install as Install).alias,
subpath,
},
mode
)
)
).then((installs) => installs.find((i) => i));
}
Expand Down Expand Up @@ -1109,18 +1121,15 @@ export class Generator {
`Adding primary constraint for ${alias}: ${JSON.stringify(target)}`
);

// Always install latest unless "freeze" is set or the user has set
// the deprecated "latest" flag explicitly:
const installLatest = this.latest ?? (this.freeze ? false : true);
await this.traceMap.add(alias, target, this.freeze, installLatest);
// By default, an install takes the latest compatible version for primary
// dependencies, and existing in-range versions for secondaries:
mode ??= "latest-primaries";

await this.traceMap.add(alias, target, mode);
await this.traceMap.visit(
alias + subpath.slice(1),
{
installOpts: {
freeze: this.freeze,
latest: installLatest,
mode: "new",
},
installMode: mode,
toplevel: true,
},
this.mapUrl.href
Expand Down Expand Up @@ -1159,13 +1168,29 @@ export class Generator {
}
}

/**
* Updates the versions of the given packages to the latest versions
* compatible with their parent's package.json ranges. If no packages are
* given then all the top-level packages in the "imports" field of the
* initial import map are updated.
*
* @param {string | string[]} pkgNames Package name or list of package names to update.
*/
async update(pkgNames?: string | string[]) {
if (typeof pkgNames === "string") pkgNames = [pkgNames];
if (this.installCnt++ === 0) this.traceMap.startInstall();
await this.traceMap.processInputMap;

const primaryResolutions = this.traceMap.installer.installs.primary;
const primaryConstraints = this.traceMap.installer.constraints.primary;
if (!pkgNames) pkgNames = Object.keys(primaryResolutions);

// Matching the behaviour of "npm update":
let mode: InstallMode = "latest-primaries";
if (!pkgNames) {
pkgNames = Object.keys(primaryResolutions);
mode = "latest-all";
}

const installs: Install[] = [];
for (const name of pkgNames) {
const resolution = primaryResolutions[name];
Expand Down Expand Up @@ -1210,7 +1235,7 @@ export class Generator {
}
}

await this.install(installs);
await this._install(installs, mode);
if (--this.installCnt === 0) {
const { map, staticDeps, dynamicDeps } =
await this.traceMap.finishInstall();
Expand Down
Loading