Skip to content
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
2 changes: 1 addition & 1 deletion actions/node/npm-split-publish/lib/pack/index.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion actions/node/npm-split-publish/lib/pack/index.js.map

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion actions/node/npm-split-publish/lib/publish/index.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion actions/node/npm-split-publish/lib/publish/index.js.map

Large diffs are not rendered by default.

22 changes: 18 additions & 4 deletions actions/node/npm-split-publish/src/registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,31 @@ export async function versionState(
cwd: string,
run: Runner,
): Promise<VersionState> {
// npm view: exit 0 + the version when it exists; exit 0 + no output when the
// package exists without that version; E404 when the package does not exist.
// npm view name@version: exit 0 + the version when it exists. When it does
// not, npm < 10 exits 0 with no output, while npm 10+ fails with E404 ("No
// match found for version"), the same code as for a package that does not
// exist at all. So on E404, ask for the package itself.
const args = ['view', `${name}@${version}`, 'version', `--registry=${registry}`];
const result = await run('npm', args, { cwd });
if (result.code === 0) {
return result.stdout.trim() !== '' ? 'published' : 'new-version';
}
if (/\bE404\b/.test(result.stderr + result.stdout)) {
if (!isNotFound(result)) {
throw new CommandError(['npm', ...args].join(' '), result);
}
const packageArgs = ['view', name, 'name', `--registry=${registry}`];
const pkg = await run('npm', packageArgs, { cwd });
if (pkg.code === 0 && pkg.stdout.trim() !== '') {
return 'new-version';
}
if (pkg.code === 0 || isNotFound(pkg)) {
return 'new-package';
}
throw new CommandError(['npm', ...args].join(' '), result);
throw new CommandError(['npm', ...packageArgs].join(' '), pkg);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Second lookup errors lack tests

The new error branch has no test. registryRunner returns E500 on the first lookup for packages in failing, so the existing error test never reaches this branch. A later change could treat a registry outage or access error as “package missing” without failing the tests.

Add a case where the version lookup returns E404 and the package lookup returns E500 or E403. Assert that versionState throws with the second command and its error.

Prompt To Fix With AI
This is a comment left during a code review.
Path: actions/node/npm-split-publish/src/registry.ts
Line: 72

Comment:
**Second lookup errors lack tests**

The new error branch has no test. `registryRunner` returns `E500` on the first lookup for packages in `failing`, so the existing error test never reaches this branch. A later change could treat a registry outage or access error as “package missing” without failing the tests.

Add a case where the version lookup returns `E404` and the package lookup returns `E500` or `E403`. Assert that `versionState` throws with the second command and its error.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Fix in Claude Code

}

function isNotFound(result: { stdout: string; stderr: string }): boolean {
return /\bE404\b/.test(result.stderr + result.stdout);
}

/** Whether name@version is in the registry (see versionState). */
Expand Down
22 changes: 17 additions & 5 deletions actions/node/npm-split-publish/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,33 @@ export const ok = (stdout = ''): RunResult => ({ code: 0, stdout, stderr: '' });
export const notFound = (): RunResult => ({ code: 1, stdout: '', stderr: 'npm error code E404\nnpm error 404 Not Found' });

/**
* Answers `npm view <name>@<version> version` from a fake registry state and
* runs everything else for real (pnpm, npm config).
* Answers `npm view <name>@<version> version` and `npm view <name> name` from a
* fake registry state the way npm 10+ does (E404 for a missing version of an
* existing package), or like npm < 10 (exit 0, no output) with `legacyNpm`,
* and runs everything else for real (pnpm, npm config).
*/
export function registryRunner(state: { published?: string[]; existing?: string[]; failing?: string[] }) {
export function registryRunner(state: { published?: string[]; existing?: string[]; failing?: string[]; legacyNpm?: boolean }) {
return fakeRunner((call) => {
if (call.command !== 'npm' || call.args[0] !== 'view') return undefined;
const spec = call.args[1];
const name = spec.slice(0, spec.lastIndexOf('@'));
const packageLookup = call.args[2] === 'name';
const name = packageLookup ? spec : spec.slice(0, spec.lastIndexOf('@'));
if (state.failing?.includes(name)) return { code: 1, stdout: '', stderr: 'npm error code E500' };
const known = state.existing?.includes(name) || state.published?.some((p) => p.startsWith(`${name}@`));
if (packageLookup) return known ? ok(name + '\n') : notFound();
if (state.published?.includes(spec)) return ok(spec.slice(spec.lastIndexOf('@') + 1) + '\n');
if (state.existing?.includes(name)) return ok('');
if (known) return state.legacyNpm ? ok('') : notFoundVersion(spec);
return notFound();
}, spawnRunner);
}

/** npm 10+ for a missing version of a package that exists. */
export const notFoundVersion = (spec: string): RunResult => ({
code: 1,
stdout: '',
stderr: `npm error code E404\nnpm error 404 No match found for version ${spec.slice(spec.lastIndexOf('@') + 1)}`,
});

export function tempDir(prefix: string): string {
return fs.mkdtempSync(path.join(os.tmpdir(), prefix));
}
Expand Down
12 changes: 12 additions & 0 deletions actions/node/npm-split-publish/test/pack.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,18 @@ describe('packWorkspace', () => {
expect(entries.map((e) => e.name)).toContain('@fixture/old');
});

it('packs a new version with npm < 10, which prints nothing for a missing version', async () => {
const { entries } = await pack({ existing: ['@fixture/old'], legacyNpm: true });
expect(entries.map((e) => e.name)).toContain('@fixture/old');
});

it('tells a new version from a new package on npmjs (npm 10+ answers E404 for both)', async () => {
const { entries, run } = await pack(ON_NPMJS, [], undefined, true);
expect(entries.map((e) => e.name)).toEqual(expect.arrayContaining(['@fixture/lib', '@fixture/app']));
const lookup = run.calls.find((c) => c.command === 'npm' && c.args[1] === '@fixture/lib' && c.args[2] === 'name');
expect(lookup?.args).toEqual(['view', '@fixture/lib', 'name', '--registry=https://registry.npmjs.org/']);
});

it('skips private packages, with or without a version, and published versions', async () => {
const { entries } = await pack();
expect(entries.map((e) => e.name)).not.toContain('@fixture/internal');
Expand Down
Loading