Skip to content

test: template pack edge cases #3497

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

Open
fredericoo wants to merge 2 commits into
base: main
Choose a base branch
from
Open

test: template pack edge cases #3497

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e79bc36
test: tempalte pack edge cases
fredericoo Feb 24, 2026
1eafcb8
refactor: get catalog straight from pnpm
fredericoo Feb 24, 2026
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
88 changes: 88 additions & 0 deletions packages/cli/src/lib/template-pack.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {execFileSync} from 'node:child_process';
import {cp, readFile, writeFile} from 'node:fs/promises';
import {join} from 'node:path';
import {describe, expect, it} from 'vitest';
Expand All @@ -12,6 +13,23 @@ const DEPENDENCY_SECTIONS = [
'optionalDependencies',
] as const;

function getCatalogVersion(sourceTemplateDir: string, packageName: string) {
const catalogVersion = execFileSync(
'pnpm',
['config', 'get', `catalog.${packageName}`, '--location', 'project'],
Copy link
Contributor Author

@fredericoo fredericoo Feb 24, 2026

Choose a reason for hiding this comment

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

TIL you can do this to get configs from pnpm and it will be monorepo aware but scoped to the right project, really cool

i was trying to do this with regexes reading the yaml, considered a yaml parser but this is even better

{
cwd: sourceTemplateDir,
encoding: 'utf8',
},
).trim();

if (!catalogVersion || catalogVersion === 'undefined') {
Copy link
Contributor

@kdaviduik kdaviduik Feb 26, 2026

Choose a reason for hiding this comment

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

nit: this === 'undefined' check might look like a bug to a future reader who doesn't know that pnpm config get returns the literal string "undefined" for missing keys. I think a one-line comment would prevent confusion:

Suggested change
if (!catalogVersion || catalogVersion === 'undefined') {
// pnpm config get returns the literal string "undefined" for missing keys
if (!catalogVersion || catalogVersion === 'undefined') {

non-blocking

throw new Error(`Expected pnpm catalog entry for ${packageName}.`);
}

return catalogVersion;
}

describe('replaceWorkspaceProtocolVersions', () => {
it('replaces workspace protocol versions in copied templates', async () => {
await inTemporaryDirectory(async (tmpDir) => {
Expand All @@ -21,6 +39,10 @@ describe('replaceWorkspaceProtocolVersions', () => {
await cp(sourceTemplateDir, copiedTemplateDir, {recursive: true});

const copiedPackageJsonPath = join(copiedTemplateDir, 'package.json');
const expectedReactVersion = getCatalogVersion(
sourceTemplateDir,
'react',
);
const copiedPackageJsonBefore = JSON.parse(
await readFile(copiedPackageJsonPath, 'utf8'),
) as Record<string, Record<string, string> | undefined>;
Expand Down Expand Up @@ -52,6 +74,72 @@ describe('replaceWorkspaceProtocolVersions', () => {
expect(version.startsWith('catalog:')).toBe(false);
Copy link
Contributor

@kdaviduik kdaviduik Feb 26, 2026

Choose a reason for hiding this comment

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

(threading) Re: line 52 - I think the comment on the @shopify/hydrogen injection should explain why this specific package was chosen. The choice is load-bearing - it must be a real workspace package that exists in the packed manifest's dependencies section for the test to work.

non-blocking

}
}

expect(copiedPackageJson.dependencies?.react).toBe(expectedReactVersion);
Copy link
Contributor

@kdaviduik kdaviduik Feb 26, 2026

Choose a reason for hiding this comment

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

I think the happy-path test should also positively assert a workspace:* resolved version.

The new react assertion is a great addition - it positively verifies the resolved value for a catalog: dependency. But the workspace:* dependencies (like @shopify/mini-oxygen in devDependencies) are only checked with negative assertions ("no workspace: prefix remains"). If the function resolved a workspace:* dep to an unexpected value, the negative assertion would still pass.

I think adding one positive spot-check would close this gap:

Suggested change
expect(copiedPackageJson.dependencies?.react).toBe(expectedReactVersion);
expect(copiedPackageJson.dependencies?.react).toBe(expectedReactVersion);
expect(copiedPackageJson.devDependencies?.['@shopify/mini-oxygen']).toMatch(
/^\d+\.\d+\.\d+/,
);

non-blocking (but in a "we definitely should do this, but i'm okay if you want to do it as a follow up PR" way)

Copy link
Contributor Author

@fredericoo fredericoo Feb 26, 2026

Choose a reason for hiding this comment

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

this sounds sensible yeah

});
});

it('throws a clear error when a workspace dependency is missing from the packed manifest', async () => {
Copy link
Contributor

@kdaviduik kdaviduik Feb 26, 2026

Choose a reason for hiding this comment

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

nit: the two error-path tests share ~18 lines of identical scaffolding (copy skeleton, read JSON, inject fake dep, write, assert rejection). At two instances this is at the boundary - I'm fine with keeping them inline since each test reads top-to-bottom as a self-contained story. But if a third error case is added (e.g., testing devDependencies or peerDependencies), I think extracting a shared helper would be clearly justified.

non-blocking

Copy link
Contributor Author

@fredericoo fredericoo Feb 26, 2026

Choose a reason for hiding this comment

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

agreed! i also draw the line at 2 hahah

await inTemporaryDirectory(async (tmpDir) => {
const sourceTemplateDir = getSkeletonSourceDir();
const copiedTemplateDir = join(tmpDir, 'skeleton-copy');

await cp(sourceTemplateDir, copiedTemplateDir, {recursive: true});

const copiedPackageJsonPath = join(copiedTemplateDir, 'package.json');
const copiedPackageJsonBefore = JSON.parse(
await readFile(copiedPackageJsonPath, 'utf8'),
) as Record<string, Record<string, string> | undefined>;

copiedPackageJsonBefore.dependencies ??= {};
copiedPackageJsonBefore.dependencies['@shopify/does-not-exist'] =
'workspace:*';

await writeFile(
copiedPackageJsonPath,
`${JSON.stringify(copiedPackageJsonBefore, null, 2)}\n`,
);

await expect(
replaceWorkspaceProtocolVersions({
sourceTemplateDir,
targetTemplateDir: copiedTemplateDir,
}),
).rejects.toThrow(
'Unable to resolve @shopify/does-not-exist from dependencies in packed template manifest.',
);
});
});

it('throws a clear error when a catalog dependency is missing from the packed manifest', async () => {
await inTemporaryDirectory(async (tmpDir) => {
const sourceTemplateDir = getSkeletonSourceDir();
const copiedTemplateDir = join(tmpDir, 'skeleton-copy');

await cp(sourceTemplateDir, copiedTemplateDir, {recursive: true});

const copiedPackageJsonPath = join(copiedTemplateDir, 'package.json');
const copiedPackageJsonBefore = JSON.parse(
await readFile(copiedPackageJsonPath, 'utf8'),
) as Record<string, Record<string, string> | undefined>;

copiedPackageJsonBefore.dependencies ??= {};
copiedPackageJsonBefore.dependencies['@shopify/catalog-does-not-exist'] =
'catalog:';

await writeFile(
copiedPackageJsonPath,
`${JSON.stringify(copiedPackageJsonBefore, null, 2)}\n`,
);

await expect(
replaceWorkspaceProtocolVersions({
sourceTemplateDir,
targetTemplateDir: copiedTemplateDir,
}),
).rejects.toThrow(
'Unable to resolve @shopify/catalog-does-not-exist from dependencies in packed template manifest.',
);
});
});
});
Loading