Skip to content

Fix custom-ocliff-loader.ts hydrogen loader strategy for hydrogen-cli local development #6827

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andguy95 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix custom-ocliff-loader.ts hydrogen loader strategy for hydrogen-cli local development #6827

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9679c32
fix: updates to new oclif v4 plugin loader strategy for hydrogen loca…
andguy95 Feb 5, 2026
2b31382
fix: variable length lint error
andguy95 Feb 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 25 additions & 57 deletions packages/cli-kit/src/public/node/custom-oclif-loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {fileExistsSync} from './fs.js'
import {cwd, joinPath, sniffForPath} from './path.js'
import {isDevelopment} from './context/local.js'
import {execaSync} from 'execa'
import {Command, Config} from '@oclif/core'
import {Config} from '@oclif/core'
Copy link
Contributor

@kdaviduik kdaviduik Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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

Copy link
Contributor

@kdaviduik kdaviduik Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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

import {Options} from '@oclif/core/interfaces'

export class ShopifyConfig extends Config {
Expand All @@ -17,6 +17,7 @@ export class ShopifyConfig extends Config {
if (currentPathMightBeHydrogenMonorepo && !ignoreHydrogenMonorepo) {
path = execaSync('npm', ['prefix']).stdout.trim()
}

if (fileExistsSync(joinPath(path, 'package.json'))) {
// Hydrogen is bundled, but we still want to support loading it as an external plugin for two reasons:
// 1. To allow users to use an older version of Hydrogen. (to not force upgrades)
Expand All @@ -25,69 +26,36 @@ export class ShopifyConfig extends Config {
core: ['@shopify/cli-hydrogen'],
path,
}
// Force OCLIF to ignore manifests so commands are loaded dynamically
// to be replaced later
options.ignoreManifest = true
Copy link
Contributor

@kdaviduik kdaviduik Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert here but several Claude Opus agents independently flagged this as dead code with a misleading comment

}
}

super(options)

if (isDevelopment()) {
// @ts-expect-error: This is a private method that we are overriding. OCLIF doesn't provide a way to extend it.
// eslint-disable-next-line @typescript-eslint/unbound-method
this.determinePriority = this.customPriority
}
}

customPriority(commands: Command.Loadable[]): Command.Loadable | undefined {
const oclifPlugins = this.pjson.oclif.plugins ?? []
const commandPlugins = commands.sort((aCommand, bCommand) => {
// eslint-disable-next-line no-restricted-syntax
const pluginAliasA = aCommand.pluginAlias ?? 'A-Cannot-Find-This'
// eslint-disable-next-line no-restricted-syntax
const pluginAliasB = bCommand.pluginAlias ?? 'B-Cannot-Find-This'
const aIndex = oclifPlugins.indexOf(pluginAliasA)
const bIndex = oclifPlugins.indexOf(pluginAliasB)

// If there is an external cli-hydrogen plugin, its commands should take priority over bundled ('core') commands
if (aCommand.pluginType === 'core' && bCommand.pluginAlias === '@shopify/cli-hydrogen') {
// If b is hydrogen and a is core sort b first
return 1
}

if (aCommand.pluginAlias === '@shopify/cli-hydrogen' && bCommand.pluginType === 'core') {
// If a is hydrogen and b is core sort a first
return -1
}

// All other cases are the default implementation from the private `determinePriority` method
// When both plugin types are 'core' plugins sort based on index
if (aCommand.pluginType === 'core' && bCommand.pluginType === 'core') {
// If b appears first in the pjson.plugins sort it first
return aIndex - bIndex
}

// if b is a core plugin and a is not sort b first
if (bCommand.pluginType === 'core' && aCommand.pluginType !== 'core') {
return 1
}

// if a is a core plugin and b is not sort a first
if (aCommand.pluginType === 'core' && bCommand.pluginType !== 'core') {
return -1
}

// if a is a jit plugin and b is not sort b first
if (aCommand.pluginType === 'jit' && bCommand.pluginType !== 'jit') {
return 1
}
async load(): Promise<void> {
await super.load()

// if b is a jit plugin and a is not sort a first
if (bCommand.pluginType === 'jit' && aCommand.pluginType !== 'jit') {
return -1
if (isDevelopment()) {
// Let OCLIF load all commands first, then manually replace bundled hydrogen
// commands with external ones after loading completes.
const externalHydrogenPlugin = Array.from(this.plugins.values()).find(
(plugin) => plugin.name === '@shopify/cli-hydrogen' && !plugin.isRoot,
)

if (externalHydrogenPlugin) {
for (const command of externalHydrogenPlugin.commands) {
if (command.id.startsWith('hydrogen')) {
Copy link
Contributor

@kdaviduik kdaviduik Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

https://www.youtube.com/watch?v=CFRhGnuXG-4

// @ts-expect-error: _commands is private but we need to replace bundled commands
if (this._commands.has(command.id)) {
Copy link
Contributor

@kdaviduik kdaviduik Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// @ts-expect-error: _commands is private but we need to replace bundled commands
this._commands.set(command.id, command)
}
}
}
}

// neither plugin is core, so do not change the order
return 0
})
return commandPlugins[0]
}
}
}
Loading