Fix CLI monorepo detection when the repo path contains spaces - #3989
Fix CLI monorepo detection when the repo path contains spaces#3989AryanKansagara wants to merge 2 commits into
Conversation
`monorepoPackagesPath` was read from `new URL(...).pathname`, which is percent-encoded. When the repository is checked out to a path containing a space, the encoded path does not exist on disk, so `isHydrogenMonorepo` is false and `getSkeletonSourceDir()` throws "Trying to use skeleton source dir outside of Hydrogen monorepo." Decode the pathname before use. `fileURLToPath` is still avoided here for the Windows backslash reasons noted in the existing comment.
fredericoo
left a comment
fredericoo
left a comment
There was a problem hiding this comment.
looks great, just please make sure the test would actually catch a regression then im happy to approve
| import {describe, it, expect} from 'vitest'; | ||
| import {decodedPathname} from './build.js'; | ||
|
|
||
| describe('decodedPathname()', () => { |
There was a problem hiding this comment.
blocking: this test can still pass if monorepo detection regresses. It only exercises decodedPathname() in isolation, so production could go back to consuming raw .pathname while both assertions remain green. Let's test the value actually consumed by monorepo detection instead, so the test fails when the production path stops decoding.
There was a problem hiding this comment.
Good catch, you're right that the old test could not have failed on that.
Detection now takes the module URL as an argument, so the test runs the real detectHydrogenMonorepo against a temporary checkout created at a path containing a space, rather than calling the decode helper directly.
I checked it fails under both regressions:
- removing
decodeURIComponentfrom the helper - leaving the helper intact but having detection consume
new URL('../../..', moduleUrl).pathnameagain, which is the case you described
Only the space assertion goes red in each; the no-space and negative cases stay green.
Full packages/cli suite passes: 57 files, 435 tests.
The previous test only called the decoding helper directly, so it stayed green if production went back to consuming raw `.pathname`. Take the module URL as an argument in `getMonorepoPackagesPath` and `detectHydrogenMonorepo` so detection can run against a real directory, and assert against a temporary checkout whose path contains a space. Verified the test fails both when the decode is removed from the helper and when detection bypasses the helper entirely.
|
@fredericoo please check again I have added a regression test |
2 participants
WHY are these changes introduced?
Fixes #3988
packages/cli/src/lib/build.tsderives the monorepo root fromimport.meta.url:URL.pathnameis percent-encoded. A checkout at/Users/me/Open Source/hydrogenproduces/Users/me/Open%20Source/hydrogen/packages/, which does not exist on disk, so theexistsSynccheck fortemplates/skeletonfails andisHydrogenMonorepobecomesfalse. Buildingpackages/clithen fails intsup.config.tswith "Trying to use skeleton source dir outside of Hydrogen monorepo", and the CLI test suite fails as well.Measured on one machine, toggling only that expression:
packages/clibuildpackages/clitestsmainThis affects contributors only. Installed npm packages are unaffected, since
isHydrogenMonorepoisfalsethere regardless. The failure is easy to misread, because the error names the monorepo layout rather than the directory name.I kept
fileURLToPathout of this path, since the existing comment says it is avoided deliberately for Windows backslash handling. Decoding the pathname preserves that while fixing the encoding.I checked for the same pattern elsewhere.
packages/hydrogen/src/vite/plugin.tsalso readsnew URL(...).pathname, but is not affected: it only testsendsWith('/packages/'), and an encoded segment always appears earlier in the path. I left it alone rather than making an unnecessary change.WHAT is this pull request doing?
decodedPathnamehelper.packages/cli/src/lib/build.test.tscovering an encoded path and a plain one. I verified the test fails if thedecodeURIComponentcall is removed.@shopify/cli-hydrogenand@shopify/create-hydrogen.build.tsis reachable from theinitpath throughlib/template-downloader.ts, so per CLAUDE.md both are bumped.The helper exists mainly so the behaviour is testable, since the original expression runs at module scope off
import.meta.urland cannot be exercised directly.HOW to test your changes?
The added unit test also covers the decoding directly:
pnpm --dir packages/cli test src/lib/build.test.tsChecklist
On cross-platform impact: Windows paths percent-encode too, so the same decode applies.
fileURLToPathwas deliberately avoided here per the existing comment, and this change keeps that. Documentation is unchecked because there is nothing user-facing to document.