Merged
Conversation
Contributor
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
itsjustriley
force-pushed
the
e2e-scaffolding-across-oses
branch
from
29691f8 to
362cda1
Compare
![binks-code-reviewer[bot]](https://avatars.githubusercontent.com/in/2788732?s=60&v=4){kind=small}
|
🤖 Code Review · #projects-dev-ai for questions ❌ Failed 📋 History✅ 1 findings → ✅ No issues → ❌ Failed |
itsjustriley
force-pushed
the
e2e-scaffolding-across-oses
branch
from
362cda1 to
b8fe3cf
Compare
itsjustriley
force-pushed
the
e2e-scaffolding-across-oses
branch
from
5d64210 to
af038a8
Compare
itsjustriley
force-pushed
the
e2e-scaffolding-across-oses
branch
from
af038a8 to
e9c7e53
Compare
Extend the test_scaffolding CI job from Linux-only to a multi-OS matrix:
Linux (npm, pnpm, yarn), macOS (npm), and Windows (npm).
Several cross-platform fixes were required to make Windows pass:
- template-pack.ts: Replace system `tar` with `tar-fs` streaming
extraction. GNU tar interprets `C:` in Windows paths as a remote host
(`host:path` rsh syntax). The streaming approach mirrors the existing
pattern in template-downloader.ts and uses only existing dependencies.
- template-pack.ts: Add `shell: true` on Windows for execFileSync
calls. Windows npm-installed CLIs are .cmd wrappers that CreateProcess
cannot execute directly — the shell must resolve them via PATH.
- template-pack.ts: Use path.isAbsolute() instead of startsWith('/')
for tarball path detection. The latter only catches Unix absolute paths
and misses Windows paths like `C:\Users\...`.
- build-check.mjs: Use fileURLToPath() instead of URL.pathname, which
produces invalid Windows paths (e.g., `/D:/a/...` with leading slash).
- hydrogen-react/package.json: Remove single quotes from cpy script
arguments. cmd.exe does not strip single quotes, so cpy-cli receives
literal quote characters in filenames and fails.
CI workflow changes:
- Explicit `shell: bash` on all steps with bash-specific syntax
- `${{ runner.temp }}` instead of `$RUNNER_TEMP` for shell-agnostic paths
- Improved lockfile verification with descriptive error messages
- Removed stale `--routes` flag (routes are now always scaffolded)
- Timeout increased 15m → 20m for slower macOS runners
- Concurrency group includes matrix.os to prevent cross-OS cancellation
Add a 60s timeout to the execFileSync('pnpm pack') call in
template-pack.ts. Without this, a hung pnpm process blocks a
developer's machine indefinitely — CI has its own 20min timeout but
local runs had no safety net.
Also add WINDOWS_SHELL_OPTS to the getCatalogVersion test helper in
template-pack.test.ts. The production code already handles Windows
shell resolution via this constant, but the test helper was missing it,
creating a latent failure if unit tests ever run on Windows.
itsjustriley
force-pushed
the
e2e-scaffolding-across-oses
branch
from
7bd0186 to
1e02160
Compare
…, portable env - Add filter to tarExtract so only package/package.json is extracted from the tarball, matching the intent of the old tar -xOf approach - Export WINDOWS_SHELL_OPTS from template-pack.ts and import in the test file to avoid duplicating the platform detection logic - Use step-level env: instead of inline VAR=value bash syntax for SHOPIFY_HYDROGEN_FLAG_LOCKFILE_CHECK, making it shell-agnostic
Contributor
Author
|
CI failures unrelated to PR |
Contributor
|
We detected some changes in |
andguy95
reviewed
Mar 20, 2026
| import {pipeline} from 'node:stream/promises'; | ||
| import {tmpdir} from 'node:os'; | ||
| import gunzipMaybe from 'gunzip-maybe'; | ||
| import {extract as tarExtract} from 'tar-fs'; |
Collaborator
There was a problem hiding this comment.
non-blocking: The PR description says "Windows doesn't ship tar". However, modern Window does ship tar.exe, and GitHub's windows-latest (Windows Server 2022) includes it.
The motivation for using tar-fs is valid though by avoiding cross-platform flag compatibility issues (BSD tar vs GNU tar) and provides a Node-native solution using deps already in this package. Worth correcting the PR description to reflect intent.
| --package-manager ${{ matrix.pm }} \ | ||
| --no-shortcut \ | ||
| --routes \ | ||
| --markets none \ |
Collaborator
There was a problem hiding this comment.
note: The --routes flag was quietly removed here. It was removed from the CLI in #3448 (routes are now auto-generated during init). Just noting for context.
The tar-fs `filter` callback received paths that never matched the
`PACKAGE_JSON_ENTRY` constant due to path normalization differences,
causing the filter to silently extract everything regardless. Rather
than fixing the path comparison, remove the filter entirely — the temp
directory is created with `mkdtemp`, cleaned up in `finally` with
`rm({recursive: true})`, and a `pnpm pack` tarball for a template is
small enough that filtering has negligible benefit.
Addresses blocking review comment on PR #3474.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WHY are these changes introduced?
Fixes developer-tools-team/issues/1044
Added macOS and Windows smoke tests to existing scaffolding tests (Linux) to ensure successful scaffolding cross-platform
What changed
template-pack.ts: Replaced systemtarCLI withtar-fsstream pipeline (Windows doesn't shiptar), addedshell: truefor Windows.cmdwrapper resolution,isAbsolute()for cross-platform path detection, andpnpm packtimeoutbuild-check.mjs: Switched fromURL.pathnametofileURLToPath()(.pathnameproduces invalid paths on Windows like/C:/Users/...)hydrogen-react/package.json: Removed quotes fromcpyglob arguments for Windowscmd.execompatibility (cmd.exe treats single quotes as literal characters rather than string delimiters)shell: bashon all steps,${{ runner.temp }}instead of$RUNNER_TEMP, improved lockfile verification error messagesCI cost note
macOS runners are ~10x Linux pricing. The macOS and Windows smoke tests use npm-only to keep costs manageable. Tests run fast so the per-PR cost delta is small, but we can look at only running this on main or conditionally later if needed.
HOW to test your changes?
CI runs this test automatically on this PR
Checklist