Fix custom-ocliff-loader.ts hydrogen loader strategy for hydrogen-cli local development #6827
Conversation
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3718 tests passing in 1438 suites. Report generated by 🧪jest coverage report action from 2b31382 |
|
Changes make sense, thank you for the pairing @andguy95. We'll want to make sure @Shopify/app-inner-loop takes a peek as they have historically made the changes to the file. |
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/custom-oclif-loader.d.ts@@ -1,6 +1,6 @@
-import { Command, Config } from '@oclif/core';
+import { Config } from '@oclif/core';
import { Options } from '@oclif/core/interfaces';
export declare class ShopifyConfig extends Config {
constructor(options: Options);
- customPriority(commands: Command.Loadable[]): Command.Loadable | undefined;
+ load(): Promise<void>;
}
\ No newline at end of file
|
| } | ||
| // Force OCLIF to ignore manifests so commands are loaded dynamically | ||
| // to be replaced later | ||
| options.ignoreManifest = true |
There was a problem hiding this comment.
I'm not an expert here but several Claude Opus agents independently flagged this as dead code with a misleading comment
| import {isDevelopment} from './context/local.js' | ||
| import {execaSync} from 'execa' | ||
| import {Command, Config} from '@oclif/core' | ||
| import {Config} from '@oclif/core' |
There was a problem hiding this comment.
(threading)
I think we should add tests here too. Otherwise we aren't fixing the true root cause, which is regressions in this file can break this functionality for the Hydrogen CLI, and no tests or CI checks will fail as a result.
I think we should add at minimum:
- A test verifying external hydrogen commands replace bundled ones after load()
- A test verifying non-dev mode does NOT trigger replacement
- A test that would catch future oclif API changes
| import {isDevelopment} from './context/local.js' | ||
| import {execaSync} from 'execa' | ||
| import {Command, Config} from '@oclif/core' | ||
| import {Config} from '@oclif/core' |
There was a problem hiding this comment.
(threading)
The PR only replaces commands by command.id. If hydrogen commands have aliases registered in _commands, those alias entries still point to bundled versions. There are currently no hydrogen command aliases (checked all 26 commands in oclif.manifest.json), so this is not a bug today but it's a latent issue.
I would recommend adding a comment documenting this limitation, or handling aliases proactively, but there might be a better way to do this overall that avoids this issue:
oclif's own insertLegacyPlugins method uses a delete-then-loadCommands pattern. I think using this.loadCommands(externalHydrogenPlugin) after deleting bundled commands would be a bit cleaner since it handles aliases and permutations through oclif's own code, though both options would involve the ts-expect-error from accessing internal properties
| for (const command of externalHydrogenPlugin.commands) { | ||
| if (command.id.startsWith('hydrogen')) { | ||
| // @ts-expect-error: _commands is private but we need to replace bundled commands | ||
| if (this._commands.has(command.id)) { |
There was a problem hiding this comment.
I think we should add in something like:
if (typeof (this as Record<string, unknown>)._commands === 'undefined') {
throw new Error('ShopifyConfig: oclif internals changed. _commands is no longer available.')
}
since _commands is a private property that Oclif may change in patch/minor versions. We don't want this to silently do nothing in that situation
|
|
||
| if (externalHydrogenPlugin) { | ||
| for (const command of externalHydrogenPlugin.commands) { | ||
| if (command.id.startsWith('hydrogen')) { |
There was a problem hiding this comment.
overall this logic has 5 layers of nesting. I think we can cut this down a bit with early returns to make this code cleaner :)
3 participants
WHY are these changes introduced?
During local development of the
@shopify/cliin theShopify/hydrogenyou are no longer able to see your changes when running thenpx shopify hydrogencommands because thecustom-ocliff-loader.tsno longer loads the localShopify/hydrgencli in theShopify/hydrogenmonorepo.These versions below are good:
In OCLIF v4,
determinePriorityis a standalone utility function (not an instance method), so we can't override it to customize command priority incustom-ocliff-loader.ts.WHAT is this pull request doing?
Change the
custom-ocliff-loader.tsto manually replace bundled hydrogen commands with external ones after loading completes if in dev mode.How to test your changes?
dev.tsinhydrogen/packages/cli/src/commands/hydrogen/dev.tsrun()cliproject runnpm run buildtemplate/skeletonproject runnpx shopify hydrogen devshopify-devpointing to a locally built version of theShopiy/clialias shopify-dev='/Users/$USER/bin/shopify'Measuring impact
How do we know this change was effective? Please choose one:
Checklist