Conversation
|
Oxygen deployed a preview of your
Learn more about Hydrogen's GitHub integration. |
isaacroldan
left a comment
There was a problem hiding this comment.
If this allows us to remove all the code to detect the hydrogen monorepo and load the plugin from the CLI repo, then great! love it!
It makes more sense for you to own this part :)
|
We detected some changes in |
| ); | ||
|
|
||
| // Marker comment is how we detect if the patch has already been applied | ||
| const MARKER = '// [hydrogen-monorepo-patch]'; |
There was a problem hiding this comment.
blocking: this MARKER constant and the ~100-line patched template literal below are duplicated nearly verbatim in packages/cli/src/lib/patch-cli.ts (generatePatchedContent()). The original content is also hardcoded in three places (unpatch-cli.mjs, patch-cli.ts, and here).
I think the scripts should import from the .ts module (compiled to .js) rather than duplicating the content inline. If the .mjs scripts need to remain standalone (for running before build), at minimum the MARKER and original content should be defined once.
This also connects to the architectural question about why we have both standalone scripts AND an importable module - and to the fact that only the .ts module has test coverage while the actually-executed .mjs script is untested. Resolving the duplication by having scripts import from the compiled module would fix both issues.
| return `#!/usr/bin/env node | ||
| ${MARKER} | ||
|
|
||
| process.removeAllListeners('warning') |
There was a problem hiding this comment.
blocking: this removes ALL warning listeners on every shopify hydrogen command invocation in the monorepo. Node.js deprecation warnings, experimental feature warnings, and security warnings are all silently swallowed.
There's no comment explaining why this is needed. If this is suppressing a specific noisy warning from oclif or Node ESM loading, it should be documented. And ideally it should use a targeted filter rather than the nuclear option:
process.on('warning', (w) => {
if (w.name !== 'SpecificWarningToSuppress') console.warn(w);
});There was a problem hiding this comment.
Added a comment above the remove listener. This just comes from the @shopify/cli/bin/run.js. Looking at the CLI repo most of the executable scripts also includes this process.removeAllListeners('warning')
I can only assume this is to remove external package dependencies who could emit noisy or irrelevant warnings that clutter the CLI output for end users
| // Since oclif v3.83.0, determinePriority is no longer an overridable | ||
| // instance method, so we manually replace the bundled commands with | ||
| // the external plugin's versions using oclif's private _commands Map. | ||
| const externalPlugin = Array.from(config.plugins.values()).find( |
There was a problem hiding this comment.
blocking: this has two silent failure paths that could cause a developer to think they're running local source when they're actually running the bundled version:
-
If
findreturnsundefinedhere (e.g.,isRootsemantics change in oclif), theif (externalPlugin)guard on line 103 silently skips all command replacement. -
If
_commandsis no longer a Map (line 105), theconsole.warnwill scroll past in CLI output while the banner still shows.
Both are the worst possible failure mode for a dev tool - the developer believes they are testing local source code when they are not. I think both should throw rather than silently continue:
if (!externalPlugin) {
throw new Error('[hydrogen-monorepo] Could not find local @shopify/cli-hydrogen plugin. The patch may need updating.');
}| } | ||
|
|
||
| /** Return the original (unpatched) file content for `run.js`. */ | ||
| export function generateOriginalContent(): string { |
There was a problem hiding this comment.
non-blocking: this hardcodes an assumed run.js format. If @shopify/cli ever changes its run.js, unpatching will overwrite it with stale content. Concrete failure scenario: pnpm install gets a new @shopify/cli with a different run.js format, then pnpm unpatch-cli overwrites it with this hardcoded version.
A safer approach would be to back up the original content before patching (e.g., write a .run.js.backup alongside it), so unpatch can restore exactly what was there.
There was a problem hiding this comment.
Good callout. Added a .backup into the directory itself where it patches (so no need for gitignore or anything).
BUt use this function as a fallback if all else goes wrong.
| "private": true, | ||
| "sideEffects": false, | ||
| "scripts": { | ||
| "patch-cli": "node scripts/patch-cli.mjs", |
There was a problem hiding this comment.
non-blocking: these are manual-only scripts with no postinstall hook. This means every developer must remember to run pnpm patch-cli after every pnpm install that refreshes node_modules. Some will forget, leading to confusing errors where their local CLI changes aren't reflected.
I think wiring patch-cli into postinstall would be the right call - it's safe because postinstall runs after all deps are installed, and the script already handles CI (process.exit(0)) and idempotency (exits if already patched).
There was a problem hiding this comment.
I initially decided to not have patches be on a post install / pre install so that devs don't get surprised by having the patched version. This way when they are ready they can opt out of the bundled experience.
But thinking about it, if you're in the monorepo, you'd probably want the patched behavior by default. Added this to prepare instead of postinstall which will patch after pnpm install finishes only in a dev context.
Also, there is a warning sign which should help determine if you're patched or not

| } | ||
|
|
||
| /** Return the full patched file content for `run.js`. */ | ||
| export function generatePatchedContent(): string { |
There was a problem hiding this comment.
question: is this module intended to be used by future shopify hydrogen patch / shopify hydrogen unpatch commands? If so, that's a nice direction and I think the standalone scripts should import from this compiled module rather than duplicating the content. If not, I'm curious why we have both the standalone scripts AND the importable module.
There was a problem hiding this comment.
I originally prototyped this as a shopify hydrogen patch/unpatch command, but realized it's not a public-facing utility. It's purely an internal developer tool for the monorepo, so I moved it to standalone scripts invoked via pnpm patch-cli/unpatch-cli.
The core logic still lives in packages/cli/src/lib/ rather than scripts/ because packages/cli has the existing TypeScript build pipeline, test infrastructure, and CI checks. Moving the items to scripts/ there would mean either giving up type safety and test coverage, or duplicating the testing and build items.
| @@ -0,0 +1,131 @@ | |||
| import {describe, it, expect} from 'vitest'; | |||
There was a problem hiding this comment.
non-blocking: these tests cover the .ts module functions well, but the actual script that runs via pnpm patch-cli (scripts/patch-cli.mjs) has its own logic (CI detection, file reading, inline content) that is untested. If the duplication in the .mjs script is resolved by importing from this compiled module, this concern goes away.
| // --- Post-load command replacement --- | ||
| // After loading, both the bundled hydrogen commands (from @shopify/cli's | ||
| // root plugin) and the local ones (from pluginAdditions) are registered. | ||
| // Since oclif v3.83.0, determinePriority is no longer an overridable |
There was a problem hiding this comment.
nit: this says "oclif v3.83.0" but I think the version 3.83.0 refers to @shopify/cli, not oclif itself. The PR description correctly says "@shopify/cli@3.83.0 (which upgraded oclif to v4)."
| // bundled plugin. The patch is version-agnostic (writes the full file rather | ||
| // than applying a diff) and idempotent. | ||
|
|
||
| import { readFileSync, writeFileSync } from 'node:fs'; |
There was a problem hiding this comment.
nit: spaces inside braces here (import { readFileSync, writeFileSync }) while packages/cli/src/lib/patch-cli.ts uses no spaces (import {readFileSync, writeFileSync}). Might be worth ensuring both follow the same style.
| } catch { | ||
| console.error( | ||
| '[patch-cli] packages/cli must be built first. Run: pnpm build --filter=@shopify/cli-hydrogen', | ||
| ); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const {applyPatch, getRunJsPath} = patchModule; | ||
| const runJsPath = getRunJsPath(ROOT); | ||
| const applied = applyPatch(runJsPath); | ||
| if (applied) console.log('[patch-cli] Patched run.js — local packages/cli will be used'); |
There was a problem hiding this comment.
blocking: this process.exit(1) will cause pnpm install to fail on a fresh clone, because prepare runs before any build has happened and before @shopify/cli is in node_modules.
There are two distinct failure modes during prepare on a fresh clone:
- Build artifacts missing - this import fails because the CLI hasn't been built yet
@shopify/clinot installed yet -applyPatchthrows becauserun.jsdoesn't exist
I think both should exit gracefully. The second catch should be targeted to avoid silently swallowing unexpected errors like permissions failures:
| } catch { | |
| console.error( | |
| '[patch-cli] packages/cli must be built first. Run: pnpm build --filter=@shopify/cli-hydrogen', | |
| ); | |
| process.exit(1); | |
| } | |
| const {applyPatch, getRunJsPath} = patchModule; | |
| const runJsPath = getRunJsPath(ROOT); | |
| const applied = applyPatch(runJsPath); | |
| if (applied) console.log('[patch-cli] Patched run.js — local packages/cli will be used'); | |
| } catch { | |
| console.warn( | |
| '[patch-cli] Skipping \u2014 packages/cli not built yet.', | |
| ); | |
| process.exit(0); | |
| } | |
| try { | |
| const {applyPatch, getRunJsPath} = patchModule; | |
| const runJsPath = getRunJsPath(ROOT); | |
| const applied = applyPatch(runJsPath); | |
| if (applied) console.log('[patch-cli] Patched run.js \u2014 local packages/cli will be used'); | |
| } catch (err) { | |
| if (err instanceof Error && err.message.includes('@shopify/cli is not installed')) { | |
| console.warn('[patch-cli] Skipping \u2014 @shopify/cli not installed yet.'); | |
| process.exit(0); | |
| } | |
| throw err; | |
| } |
| * hardcoded original content. | ||
| */ | ||
| export function removePatch(runJsPath: string): boolean { | ||
| const current = readFileSync(runJsPath, 'utf8'); |
There was a problem hiding this comment.
nit: removePatch calls readFileSync bare here, but applyPatch handles the ENOENT case gracefully with '@shopify/cli is not installed'. If someone runs pnpm unpatch-cli after a clean node_modules wipe, this throws a raw stack trace. I think adding similar error handling would be cleaner for consistency.
| export function generateOriginalContent(): string { | ||
| return `#!/usr/bin/env node | ||
|
|
||
| // process.removeAllListeners('warning') |
There was a problem hiding this comment.
non-blocking: this line is commented out but the actual upstream run.js has it uncommented. If the .backup is ever lost and this fallback fires, the restored run.js would behave differently from the real upstream.
Related: the test 'preserves upstream removeAllListeners call' passes because toContain matches the substring even inside this comment - the test name implies the call is active but it's actually inert. I think uncommenting this line fixes both the behavioral mismatch and the misleading test.
| try { | ||
| current = readFileSync(runJsPath, 'utf8'); | ||
| } catch (err: unknown) { | ||
| if ((err as NodeJS.ErrnoException).code === 'ENOENT') { |
There was a problem hiding this comment.
non-blocking: I think a type guard would be slightly safer than as NodeJS.ErrnoException here since err is unknown, I really like to avoid as at all costs:
function isErrnoException(err: unknown): err is NodeJS.ErrnoException {
return err instanceof Error && 'code' in err;
}
kdaviduik
left a comment
There was a problem hiding this comment.
Thanks Andrew! Changes LGTM, although I definitely think we need docs updates to go with it (follow up PR).
One thing not in the inline comments: CLAUDE.md's "CLI Dependency Graph" section should mention the development-time patch override. Specifically: that run.js is patched in development, that it happens via prepare, that IGNORE_HYDROGEN_MONOREPO=1 disables ShopifyConfig's own detection, and that _commands is a private oclif API (the most likely breakage point on oclif upgrades). Also feel free to make a new skill instead of putting it in the CLAUDE.md
| let patchModule; | ||
| try { | ||
| patchModule = await import('../packages/cli/dist/lib/patch-cli.js'); | ||
| } catch { |
There was a problem hiding this comment.
non-blocking: on a fresh clone, pnpm install triggers prepare before packages/cli is built, so this catch fires and the patch doesn't apply. After building (pnpm build --filter=@shopify/cli-hydrogen), the developer has to manually run pnpm patch-cli or pnpm install again. The banner's absence is the signal, but new contributors might not realize their first install didn't patch.
| "prepare": "husky", | ||
| "patch-cli": "node scripts/patch-cli.mjs", | ||
| "unpatch-cli": "node scripts/unpatch-cli.mjs", | ||
| "prepare": "husky && node scripts/patch-cli.mjs", |
There was a problem hiding this comment.
non-blocking: this prepare hook silently modifies node_modules/@shopify/cli/bin/run.js on every pnpm install. I think we should have docs that mention the patch system - what the orange banner means, that it runs automatically via prepare, and how to manually apply/remove it (pnpm patch-cli / pnpm unpatch-cli).
| ); | ||
| } | ||
|
|
||
| const cmds = config._commands; |
There was a problem hiding this comment.
nit: _commands is an oclif private API (underscore prefix). This is the same category of coupling that caused the problem this PR solves (oclif v4 changed determinePriority from an instance method to a standalone function). The runtime guard is the right defensive approach - I think adding a comment noting this is a private API that should be verified on oclif upgrades would help future maintainers.
| /** | ||
| * Last-resort fallback content for run.js. | ||
| * Prefer restoring from .backup file (see removePatch). | ||
| * This may drift from upstream — update when bumping @shopify/cli. |
There was a problem hiding this comment.
nit: I think adding the specific @shopify/cli version this was synced from would help maintainers know when this fallback needs updating. Something like "Synced with @shopify/cli@3.85.4" - the backup mechanism means this rarely fires, but when it does, version provenance matters.
frandiox
left a comment
There was a problem hiding this comment.
Amazing! Much better here than in the CLI repo 🙌
Co-authored-by: Kara Daviduik <105449131+kdaviduik@users.noreply.github.com>

WHY are these changes introduced?
Replaces: Shopify/cli#6827
Since
@shopify/cli@3.83.0(which upgraded oclif to v4), runningshopify hydrogencommands in the Hydrogen monorepo no longer loads the localpackages/clisource code. Instead, the bundled version inside@shopify/cli/distis always used, making it impossible to test local CLI changes during development.Root cause: The previous mechanism in
@shopify/cli-kit'sShopifyConfigrelied on overriding oclif'sdeterminePriorityinstance method to make external plugin commands win over bundled ones. In oclif v4 (shipped in@shopify/cli@3.83.0),determinePrioritybecame a standalone utility function, so the override silently stopped working. Additionally, the monorepo detection itself is not the best:/(shopify|hydrogen)\/hydrogen/itested againstcwd()to guess if you're in the monorepoexecSync('npm', ['prefix'])on every CLI invocation to find the project rootdeterminePrioritymethodThis PR introduces a self-contained patch system within the Hydrogen monorepo that replaces all of that.
WHAT is this pull request doing?
Adds two scripts that enable local CLI development:
scripts/patch-cli.mjs— Patchesnode_modules/@shopify/cli/bin/run.jsto load the local@shopify/cli-hydrogenplugin frompackages/cliinstead of the bundled version. The patch:cwd()looking forpackages/cli/package.json— no regex, no subprocessespluginAdditionsmechanism to register the local@shopify/cli-hydrogenas a core plugin, the same mechanismShopifyConfiguses internally_commandsMap and re-inserting them from the local plugin vialoadCommands()— this is the same strategy proposed in the upstream CLI fix (Fix custom-oclif-loader.ts hydrogen loader strategy for hydrogen-cli local development cli#6827) and works with oclif v4IGNORE_HYDROGEN_MONOREPO=1so the currentShopifyConfigin@shopify/cli-kitdoesn't attempt its own (broken) monorepo detection on top (NOTE: We can remove this if we update thecustom-oclif-loaderto become purely for letting users overridecli-hydrogenwith a different version from their project'snode_modules.)patch-packagesorpnpm patchscripts/unpatch-cli.mjs— Restoresrun.jsto its original content. Also idempotent.How the patch interacts with the CLI plugin architecture:
For non-hydrogen commands or when running outside the monorepo, the standard
@shopify/clientrypoint is used unchanged.Upstream impact: This patch is can replace Shopify/cli#6827, which fixes the
custom-oclif-loaderfor local@shopify/cli-hydrogendevelopment in the monorepo. Thecustom-oclif-loadercan be simplified to remove all monorepo-specific logic (regex,npm prefix,IGNORE_HYDROGEN_MONOREPO), since this patch handles that case entirely within the Hydrogen repo.HOW to test your changes?
pnpm install(orpnpm patch-cliif already installed)packages/cli/src/commands/hydrogen/dev.ts(e.g. add aconsole.log('test')in therun()method)pnpm run build --filter=@shopify/cli-hydrogentemplates/skeleton, runnpx shopify hydrogen devhydrogen-monorepobanner appearsconsole.logfrom step 2 is printedpnpm unpatch-cliand repeat step 4 — the banner should not appear and your change should not be visible (bundled version is used)Checklist