Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
| function getCatalogVersion(sourceTemplateDir: string, packageName: string) { | ||
| const catalogVersion = execFileSync( | ||
| 'pnpm', | ||
| ['config', 'get', `catalog.${packageName}`, '--location', 'project'], |
There was a problem hiding this comment.
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
|
We detected some changes in |
| } | ||
| } | ||
|
|
||
| expect(copiedPackageJson.dependencies?.react).toBe(expectedReactVersion); |
There was a problem hiding this comment.
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:
| 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)
There was a problem hiding this comment.
this sounds sensible yeah
| }); | ||
| }); | ||
|
|
||
| it('throws a clear error when a workspace dependency is missing from the packed manifest', async () => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
agreed! i also draw the line at 2 hahah
| }, | ||
| ).trim(); | ||
|
|
||
| if (!catalogVersion || catalogVersion === 'undefined') { |
There was a problem hiding this comment.
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:
| if (!catalogVersion || catalogVersion === 'undefined') { | |
| // pnpm config get returns the literal string "undefined" for missing keys | |
| if (!catalogVersion || catalogVersion === 'undefined') { |
non-blocking
| @@ -52,6 +74,72 @@ describe('replaceWorkspaceProtocolVersions', () => { | |||
| expect(version.startsWith('catalog:')).toBe(false); | |||
There was a problem hiding this comment.
(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
2 participants
WHY are these changes introduced?
replaceWorkspaceProtocolVersionsruns in the template packaging path used for releases. The existing integration coverage verified thatworkspace:andcatalog:prefixes were removed, but it did not verify exactcatalog:resolution or the error behavior when protocol-based dependencies cannot be found in the packed manifest.WHAT is this pull request doing?
template-pack.test.tsto read a dependency version from thecatalogsection ofpnpm-workspace.yaml.react(fromcatalog) instead of only checking protocol prefix removal.workspace:*dependency is missing from the packed manifest.catalog:dependency is missing from the packed manifest.HOW to test your changes?
cd packages/clipnpm exec vitest run src/lib/template-pack.test.ts3tests in this file)Post-merge steps
None.
Checklist