Skip to content

Commit 9803e6f

Browse files
committed
fix(@angular/cli): refine min-release-age version selection
Follow-up to the previous commit, addressing review feedback. - Skip the metadata fetch for explicit version specifiers when a cooldown is configured. The package manager itself enforces the cooldown at install time, and an extra registry round-trip just slows down the common case (`ng add foo@1.2.3`). - Pass `includePrerelease: true` to `maxSatisfying` on the tag fallback path so `next` does not silently downgrade to a stable release when its current pointer is too new. - Detect a `.git` repo root by existence rather than `isDirectory()` so the `.npmrc` walk also stops at submodule and worktree roots, where `.git` is a regular file containing a `gitdir:` pointer. New unit tests cover the prerelease fallback and the gitfile case.
1 parent b9c1ec7 commit 9803e6f

4 files changed

Lines changed: 93 additions & 17 deletions

File tree

packages/angular/cli/src/package-managers/min-release-age.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,12 @@ async function readNpmrcChain(
6767
directoriesToVisit.push(currentDir);
6868

6969
// Stop walking when we reach a git repository root, mirroring `discovery.ts`.
70+
// `.git` may be a directory (regular repo), a file (submodules and
71+
// worktrees use a `gitdir:` pointer file), or absent. We just need to
72+
// know whether it exists; `host.stat` rejects when it doesn't.
7073
try {
71-
if ((await host.stat(join(currentDir, '.git'))).isDirectory()) {
72-
break;
73-
}
74+
await host.stat(join(currentDir, '.git'));
75+
break;
7476
} catch {
7577
// No `.git` here; continue searching upwards.
7678
}

packages/angular/cli/src/package-managers/min-release-age_spec.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,18 @@ describe('getMinReleaseAgeMs', () => {
7777
).toBe(2 * MS_PER_DAY);
7878
});
7979

80+
it('treats a `.git` file as a repo root (git submodules and worktrees)', async () => {
81+
const host = new MockHost();
82+
// In a submodule or worktree `.git` is a regular file containing a
83+
// `gitdir:` pointer, not a directory. The walk must still stop here.
84+
host.setFile('/repo/.git', 'gitdir: /elsewhere/.git/modules/repo\n');
85+
host.setFile('/repo/.npmrc', 'min-release-age=4\n');
86+
87+
expect(await getMinReleaseAgeMs(host, '/repo', SUPPORTED_PACKAGE_MANAGERS.npm)).toBe(
88+
4 * MS_PER_DAY,
89+
);
90+
});
91+
8092
it('returns 0 for non-positive or non-numeric values', async () => {
8193
const host = new MockHost();
8294
host.setDirectory('/project/.git');

packages/angular/cli/src/package-managers/package-manager.ts

Lines changed: 21 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -534,14 +534,14 @@ export class PackageManager {
534534

535535
// We only need to resolve the version locally when:
536536
// 1. The package manager requires it (e.g. yarn-classic), OR
537-
// 2. A `minReleaseAge` cooldown is configured. In that case, even for
538-
// `range`/`tag` specifiers we must filter out versions newer than
539-
// the cutoff; otherwise the package manager would refuse to install
540-
// the version we picked.
537+
// 2. A `minReleaseAge` cooldown is configured AND the spec is a `tag`
538+
// or `range`. For an explicit version we skip the extra metadata
539+
// lookup; if the version is too new the package manager itself
540+
// will reject the install with a clearer error than we could give.
541541
const needsLocalResolution =
542542
(this.descriptor.requiresManifestVersionLookup &&
543543
(type === 'tag' || type === 'range' || !fetchSpec)) ||
544-
minReleaseAgeMs > 0;
544+
(minReleaseAgeMs > 0 && (type === 'tag' || type === 'range'));
545545

546546
if (needsLocalResolution) {
547547
const metadata = await this.getRegistryMetadata(name, options);
@@ -613,15 +613,19 @@ export class PackageManager {
613613
* release age cooldown.
614614
*
615615
* Behavior summary:
616-
* - `version`: returns the requested version, or `null` when it is too new.
617-
* The caller asked for an exact version, so we don't second-guess them.
618616
* - `tag`: resolves the tag, then returns it directly when it is old enough.
619617
* When it is too new, falls back to the newest version that satisfies the
620618
* cooldown, so commands like `ng update` and `ng add` can still proceed.
621619
* - `range`: returns the newest version in the range that is old enough.
622620
*
621+
* Note: this is not called for explicit `version` specifiers when a
622+
* cooldown is configured (the manifest endpoint is used directly and the
623+
* package manager enforces the cooldown at install time).
624+
*
623625
* @param metadata The package metadata returned by the registry.
624-
* @param type The specifier type (`tag`, `range`, or `version`).
626+
* @param type The specifier type (`tag` or `range`). `version` is only
627+
* passed when `requiresManifestVersionLookup` is set, in which case the
628+
* cooldown is `0` and we just echo the version back.
625629
* @param spec The tag, range, or version requested by the caller.
626630
* @param minReleaseAgeMs The cooldown in milliseconds. `0` disables filtering.
627631
* @returns A concrete version string, or `null` when no version qualifies.
@@ -664,10 +668,15 @@ export class PackageManager {
664668
return Number.isNaN(publishedMs) ? true : publishedMs <= cutoff;
665669
};
666670

671+
// Pass `includePrerelease` so prereleases are eligible. This matters when
672+
// the resolved tag points at a prerelease (e.g. `next`); otherwise the
673+
// fallback would silently downgrade to a stable release.
674+
const semverOptions = { includePrerelease: true };
675+
667676
if (type === 'range') {
668677
const eligible = metadata.versions.filter(isOldEnough);
669678

670-
return maxSatisfying(eligible, spec) ?? null;
679+
return maxSatisfying(eligible, spec, semverOptions) ?? null;
671680
}
672681

673682
if (isOldEnough(spec)) {
@@ -679,10 +688,11 @@ export class PackageManager {
679688
// that satisfies the cooldown so the command can still complete.
680689
const eligible = metadata.versions.filter(isOldEnough);
681690

682-
return maxSatisfying(eligible, '*') ?? null;
691+
return maxSatisfying(eligible, '*', semverOptions) ?? null;
683692
}
684693

685-
// Exact version requested but it's too new: signal "no installable version".
694+
// Should not be reachable: callers no longer pass an explicit `version`
695+
// with a cooldown configured. Returning `null` is the safe default.
686696
return null;
687697
}
688698

packages/angular/cli/src/package-managers/package-manager_spec.ts

Lines changed: 55 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -188,20 +188,27 @@ describe('PackageManager', () => {
188188
expect(manifest?.version).toBe('21.3.0');
189189
});
190190

191-
it('returns null when an explicit version is younger than the cooldown', async () => {
191+
it('does not fetch metadata for an explicit version even when a cooldown is configured', async () => {
192192
spyOn(host, 'readFile').and.callFake((path: string) => {
193193
if (path === '/tmp/.npmrc') {
194194
return Promise.resolve('min-release-age=7\n');
195195
}
196196

197197
return Promise.reject(new Error('not found'));
198198
});
199-
mockMetadataResponse();
199+
runCommandSpy.and.resolveTo({
200+
stdout: JSON.stringify({ name: 'foo', version: '21.4.0' }),
201+
stderr: '',
202+
});
200203

201204
const pm = new PackageManager(host, '/tmp', descriptor);
202205
const manifest = await pm.getManifest('foo@21.4.0');
203206

204-
expect(manifest).toBeNull();
207+
// Hit `getRegistryManifest` directly. The package manager enforces the
208+
// cooldown at install time; the CLI should not second-guess an explicit
209+
// version, since that would block legitimate use cases like pinning.
210+
expect(manifest?.version).toBe('21.4.0');
211+
expect(runCommandSpy).toHaveBeenCalledTimes(1);
205212
});
206213

207214
it('does not call `getRegistryMetadata` when no cooldown is configured', async () => {
@@ -218,5 +225,50 @@ describe('PackageManager', () => {
218225
// A single call: directly to `getRegistryManifest`. No metadata lookup.
219226
expect(runCommandSpy).toHaveBeenCalledTimes(1);
220227
});
228+
229+
it('keeps prereleases eligible when falling back from a prerelease tag', async () => {
230+
spyOn(host, 'readFile').and.callFake((path: string) => {
231+
if (path === '/tmp/.npmrc') {
232+
return Promise.resolve('min-release-age=7\n');
233+
}
234+
235+
return Promise.reject(new Error('not found'));
236+
});
237+
238+
const metadata = {
239+
name: 'foo',
240+
'dist-tags': { next: '22.0.0-next.5', latest: '21.3.0' },
241+
versions: ['21.3.0', '22.0.0-next.4', '22.0.0-next.5'],
242+
time: {
243+
// Stable older release.
244+
'21.3.0': new Date(now - 30 * MS_PER_DAY).toISOString(),
245+
// Older prerelease, eligible.
246+
'22.0.0-next.4': new Date(now - 15 * MS_PER_DAY).toISOString(),
247+
// Newest prerelease, blocked by the cooldown.
248+
'22.0.0-next.5': new Date(now - 1 * MS_PER_DAY).toISOString(),
249+
},
250+
};
251+
252+
runCommandSpy.and.callFake((_binary: string, args: readonly string[]) => {
253+
const versionedSpec = args.find((a) => /^foo@\S+/.test(a));
254+
if (!versionedSpec) {
255+
return Promise.resolve({ stdout: JSON.stringify(metadata), stderr: '' });
256+
}
257+
258+
const requestedVersion = /^foo@(\S+)/.exec(versionedSpec)?.[1] ?? '';
259+
260+
return Promise.resolve({
261+
stdout: JSON.stringify({ name: 'foo', version: requestedVersion }),
262+
stderr: '',
263+
});
264+
});
265+
266+
const pm = new PackageManager(host, '/tmp', descriptor);
267+
const manifest = await pm.getManifest('foo@next');
268+
269+
// Should pick the older prerelease, not silently downgrade to a stable
270+
// version that the user never asked for.
271+
expect(manifest?.version).toBe('22.0.0-next.4');
272+
});
221273
});
222274
});

0 commit comments

Comments
 (0)