diff --git a/.changeset/pretty-swans-smell.md b/.changeset/pretty-swans-smell.md new file mode 100644 index 0000000000..3a46456c03 --- /dev/null +++ b/.changeset/pretty-swans-smell.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': patch +--- + +Fix hydrogen local dev plugin loader to correctly replace bundled hydrogen commands with external plugin commands by switching to a post-load strategy. Also removes the now-dead `customPriority` method from `ShopifyConfig` (it was non-functional since oclif v3.83.0 and had no callers); the public type declaration for `custom-oclif-loader.js` reflects this removal. diff --git a/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts b/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts index 688d4605f4..9bbf33dd86 100644 --- a/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts +++ b/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts @@ -86,6 +86,7 @@ export type OrganizationUserProvisionShopAccessInput = { export type Store = | 'APP_DEVELOPMENT' | 'CLIENT_TRANSFER' + | 'COLLABORATOR' | 'DEVELOPMENT' | 'DEVELOPMENT_SUPERSET' | 'PRODUCTION'; diff --git a/packages/cli-kit/src/public/node/custom-oclif-loader.test.ts b/packages/cli-kit/src/public/node/custom-oclif-loader.test.ts new file mode 100644 index 0000000000..267c9bc06c --- /dev/null +++ b/packages/cli-kit/src/public/node/custom-oclif-loader.test.ts @@ -0,0 +1,341 @@ +import {ShopifyConfig} from './custom-oclif-loader.js' +import {isDevelopment} from './context/local.js' +import {fileExistsSync} from './fs.js' +import {cwd, joinPath, sniffForPath} from './path.js' +import {execaSync} from 'execa' +import {describe, test, expect, vi, beforeEach} from 'vitest' +import {fileURLToPath} from 'node:url' +import type {Config as OclifConfig} from '@oclif/core' +import type {Options} from '@oclif/core/interfaces' + +vi.mock('./context/local.js') +vi.mock('./fs.js') +vi.mock('./path.js') +vi.mock('execa') + +// Provide a controllable base class so tests can inspect `plugins`, `_commands`, +// and whether `loadCommands` was invoked without depending on real oclif internals. +vi.mock('@oclif/core', () => { + class Config { + plugins = new Map() + _commands = new Map() + loadCommandsCalls: unknown[] = [] + + async load(): Promise {} + + loadCommands(plugin: unknown): void { + this.loadCommandsCalls.push(plugin) + } + } + + return {Config} +}) + +// Convenience type so tests can reach mock-only properties without ts-expect-error on every line. +interface MockConfig { + plugins: Map + _commands: Map | undefined + loadCommandsCalls: unknown[] +} + +function asMock(config: ShopifyConfig): MockConfig { + return config as unknown as MockConfig +} + +describe('ShopifyConfig', () => { + beforeEach(() => { + vi.mocked(isDevelopment).mockReturnValue(false) + vi.mocked(cwd).mockReturnValue('/workspace') + vi.mocked(sniffForPath).mockReturnValue(undefined) + vi.mocked(joinPath).mockImplementation((...args: string[]) => args.join('/')) + vi.mocked(fileExistsSync).mockReturnValue(false) + delete process.env.IGNORE_HYDROGEN_MONOREPO + }) + + describe('constructor', () => { + test('does not set pluginAdditions when not in dev mode', () => { + const options = {root: '/workspace'} as Options + const _config = new ShopifyConfig(options) + expect((options as {pluginAdditions?: unknown}).pluginAdditions).toBeUndefined() + expect(options.ignoreManifest).toBeUndefined() + }) + + test('sets pluginAdditions but not ignoreManifest when package.json exists outside H2 monorepo in dev mode', () => { + vi.mocked(isDevelopment).mockReturnValue(true) + vi.mocked(fileExistsSync).mockReturnValue(true) + + const options = {root: '/workspace'} as Options + const _config = new ShopifyConfig(options) + + expect((options as {pluginAdditions?: unknown}).pluginAdditions).toEqual({ + core: ['@shopify/cli-hydrogen'], + path: '/workspace', + }) + expect(options.ignoreManifest).toBeUndefined() + }) + + test('sets pluginAdditions and ignoreManifest when package.json exists inside H2 monorepo in dev mode', () => { + vi.mocked(isDevelopment).mockReturnValue(true) + vi.mocked(cwd).mockReturnValue('/home/user/shopify/hydrogen/packages/cli') + vi.mocked(execaSync).mockReturnValue({stdout: '/home/user/shopify/hydrogen'} as unknown as ReturnType< + typeof execaSync + >) + vi.mocked(fileExistsSync).mockReturnValue(true) + + const options = {root: '/workspace'} as Options + const _config = new ShopifyConfig(options) + + expect((options as {pluginAdditions?: unknown}).pluginAdditions).toMatchObject({ + core: ['@shopify/cli-hydrogen'], + path: '/home/user/shopify/hydrogen', + }) + expect(options.ignoreManifest).toBe(true) + }) + + test('does not set pluginAdditions when package.json is absent in dev mode', () => { + vi.mocked(isDevelopment).mockReturnValue(true) + vi.mocked(fileExistsSync).mockReturnValue(false) + + const options = {root: '/workspace'} as Options + const _config = new ShopifyConfig(options) + + expect((options as {pluginAdditions?: unknown}).pluginAdditions).toBeUndefined() + }) + + test('uses sniffForPath result over cwd when available', () => { + vi.mocked(isDevelopment).mockReturnValue(true) + vi.mocked(sniffForPath).mockReturnValue('/sniffed/path') + vi.mocked(fileExistsSync).mockReturnValue(true) + + const options = {root: '/workspace'} as Options + const _config = new ShopifyConfig(options) + + expect((options as {pluginAdditions?: unknown}).pluginAdditions).toMatchObject({path: '/sniffed/path'}) + }) + + test('runs npm prefix when cwd matches hydrogen monorepo pattern', () => { + vi.mocked(isDevelopment).mockReturnValue(true) + vi.mocked(cwd).mockReturnValue('/home/user/shopify/hydrogen/packages/cli') + vi.mocked(execaSync).mockReturnValue({stdout: '/home/user/shopify/hydrogen'} as unknown as ReturnType< + typeof execaSync + >) + vi.mocked(fileExistsSync).mockReturnValue(true) + + const options = {root: '/workspace'} as Options + const _config = new ShopifyConfig(options) + + expect(execaSync).toHaveBeenCalledWith('npm', ['prefix']) + expect((options as {pluginAdditions?: unknown}).pluginAdditions).toMatchObject({ + path: '/home/user/shopify/hydrogen', + }) + }) + + test('also matches hydrogen/hydrogen CI path pattern', () => { + vi.mocked(isDevelopment).mockReturnValue(true) + vi.mocked(cwd).mockReturnValue('/runner/hydrogen/hydrogen/packages/cli') + vi.mocked(execaSync).mockReturnValue({stdout: '/runner/hydrogen/hydrogen'} as unknown as ReturnType< + typeof execaSync + >) + vi.mocked(fileExistsSync).mockReturnValue(true) + + const _config = new ShopifyConfig({root: '/workspace'} as Options) + + expect(execaSync).toHaveBeenCalledWith('npm', ['prefix']) + }) + + test('skips npm prefix and ignoreManifest when IGNORE_HYDROGEN_MONOREPO is set', () => { + vi.mocked(isDevelopment).mockReturnValue(true) + vi.mocked(cwd).mockReturnValue('/home/user/shopify/hydrogen/packages/cli') + vi.mocked(fileExistsSync).mockReturnValue(true) + process.env.IGNORE_HYDROGEN_MONOREPO = '1' + + const options = {root: '/workspace'} as Options + const _config = new ShopifyConfig(options) + + expect(execaSync).not.toHaveBeenCalled() + expect(options.ignoreManifest).toBeUndefined() + }) + }) + + describe('load()', () => { + test('does not replace commands when not in dev mode', async () => { + vi.mocked(isDevelopment).mockReturnValue(false) + + const config = new ShopifyConfig({root: '/workspace'} as Options) + const hydrogenPlugin = { + name: '@shopify/cli-hydrogen', + isRoot: false, + commands: [{id: 'hydrogen:dev', aliases: [], hiddenAliases: []}], + } + asMock(config).plugins.set('@shopify/cli-hydrogen', hydrogenPlugin) + asMock(config)._commands!.set('hydrogen:dev', {bundled: true}) + + await config.load() + + expect(asMock(config)._commands!.has('hydrogen:dev')).toBe(true) + expect(asMock(config).loadCommandsCalls).toHaveLength(0) + }) + + test('does not replace commands when no external hydrogen plugin is present', async () => { + vi.mocked(isDevelopment).mockReturnValue(true) + + const config = new ShopifyConfig({root: '/workspace'} as Options) + asMock(config)._commands!.set('hydrogen:dev', {bundled: true}) + + await config.load() + + expect(asMock(config)._commands!.has('hydrogen:dev')).toBe(true) + expect(asMock(config).loadCommandsCalls).toHaveLength(0) + }) + + test('does not replace commands when the hydrogen plugin is the root plugin', async () => { + vi.mocked(isDevelopment).mockReturnValue(true) + + const config = new ShopifyConfig({root: '/workspace'} as Options) + asMock(config).plugins.set('@shopify/cli-hydrogen', {name: '@shopify/cli-hydrogen', isRoot: true, commands: []}) + asMock(config)._commands!.set('hydrogen:dev', {bundled: true}) + + await config.load() + + expect(asMock(config)._commands!.has('hydrogen:dev')).toBe(true) + expect(asMock(config).loadCommandsCalls).toHaveLength(0) + }) + + test('removes bundled hydrogen commands and reloads from external plugin', async () => { + vi.mocked(isDevelopment).mockReturnValue(true) + + const config = new ShopifyConfig({root: '/workspace'} as Options) + const externalPlugin = { + name: '@shopify/cli-hydrogen', + isRoot: false, + commands: [ + {id: 'hydrogen:dev', aliases: ['h:dev'], hiddenAliases: ['hydrogen:develop']}, + {id: 'hydrogen:build', aliases: [], hiddenAliases: undefined}, + ], + } + asMock(config).plugins.set('@shopify/cli-hydrogen', externalPlugin) + + // Populate _commands with bundled versions of hydrogen commands plus an unrelated one + asMock(config)._commands!.set('hydrogen:dev', {bundled: true}) + asMock(config)._commands!.set('h:dev', {bundled: true}) + asMock(config)._commands!.set('hydrogen:develop', {bundled: true}) + asMock(config)._commands!.set('hydrogen:build', {bundled: true}) + asMock(config)._commands!.set('app:dev', {bundled: true}) + + await config.load() + + // All bundled hydrogen entries (canonical + aliases + hidden aliases) are gone + expect(asMock(config)._commands!.has('hydrogen:dev')).toBe(false) + expect(asMock(config)._commands!.has('h:dev')).toBe(false) + expect(asMock(config)._commands!.has('hydrogen:develop')).toBe(false) + expect(asMock(config)._commands!.has('hydrogen:build')).toBe(false) + + // Non-hydrogen commands are untouched + expect(asMock(config)._commands!.has('app:dev')).toBe(true) + + // loadCommands is called exactly once with the external plugin + expect(asMock(config).loadCommandsCalls).toHaveLength(1) + expect(asMock(config).loadCommandsCalls[0]).toBe(externalPlugin) + }) + + test('only removes commands whose id starts with "hydrogen"', async () => { + vi.mocked(isDevelopment).mockReturnValue(true) + + const config = new ShopifyConfig({root: '/workspace'} as Options) + const externalPlugin = { + name: '@shopify/cli-hydrogen', + isRoot: false, + // A non-hydrogen-prefixed command shipped by the hydrogen plugin + commands: [{id: 'app:generate:route', aliases: [], hiddenAliases: []}], + } + asMock(config).plugins.set('@shopify/cli-hydrogen', externalPlugin) + asMock(config)._commands!.set('app:generate:route', {bundled: true}) + + await config.load() + + // The command is not hydrogen-prefixed so it must not be removed + expect(asMock(config)._commands!.has('app:generate:route')).toBe(true) + }) + + test('throws a descriptive error when _commands is unavailable, catching future oclif API changes', async () => { + vi.mocked(isDevelopment).mockReturnValue(true) + + const config = new ShopifyConfig({root: '/workspace'} as Options) + asMock(config).plugins.set('@shopify/cli-hydrogen', { + name: '@shopify/cli-hydrogen', + isRoot: false, + commands: [], + }) + + // Simulate oclif removing the private _commands property + asMock(config)._commands = undefined + + await expect(config.load()).rejects.toThrow( + 'ShopifyConfig: oclif internals changed. _commands is no longer available.', + ) + }) + + test('throws a descriptive error when _commands is not a Map, catching future oclif API changes', async () => { + vi.mocked(isDevelopment).mockReturnValue(true) + + const config = new ShopifyConfig({root: '/workspace'} as Options) + asMock(config).plugins.set('@shopify/cli-hydrogen', { + name: '@shopify/cli-hydrogen', + isRoot: false, + commands: [], + }) + + // Simulate oclif changing _commands from Map to a plain object + ;(config as unknown as {_commands: unknown})._commands = {} + + await expect(config.load()).rejects.toThrow( + 'ShopifyConfig: oclif internals changed. _commands is no longer a Map.', + ) + }) + }) + + // These tests use the REAL @oclif/core (via vi.importActual) so they will fail + // if oclif removes or renames the private APIs that ShopifyConfig depends on. + // The mock above intentionally replaces oclif for logic isolation; this block + // provides the missing contract check against the installed package version. + describe('oclif API contract', () => { + test('Config still has a loadCommands method on its prototype', async () => { + const {Config: RealConfig} = await vi.importActual('@oclif/core') + + // ShopifyConfig calls this.loadCommands(plugin) via @ts-expect-error. + // If oclif removes or renames this method, this assertion will catch it. + expect(typeof (RealConfig as unknown as {prototype: {[key: string]: unknown}}).prototype.loadCommands).toBe( + 'function', + ) + }) + + test('Config instances still have a _commands own property after construction', async () => { + const {Config: RealConfig} = await vi.importActual('@oclif/core') + + // _commands is a class field initialized in the constructor, so it appears as an + // own property on every instance even before load() is called. + // ShopifyConfig reads and mutates this._commands as a Map — if oclif renames or + // restructures it, this assertion will fail. + const instance = new (RealConfig as new (options: {root: string}) => OclifConfig)({ + root: fileURLToPath(new URL('.', import.meta.url)), + }) + expect(Object.prototype.hasOwnProperty.call(instance, '_commands')).toBe(true) + }) + + test('Command objects still expose hiddenAliases', async () => { + const {Config: RealConfig} = await vi.importActual('@oclif/core') + + // ShopifyConfig reads hiddenAliases from command objects via an OclifCommand cast. + // If oclif renames or removes this property, the ?? [] fallback silently skips alias + // cleanup, leaving stale bundled entries in _commands. This assertion catches that. + const config = new (RealConfig as new (options: {root: string}) => OclifConfig)({ + root: fileURLToPath(new URL('.', import.meta.url)), + }) + await config.load() + const someCommand = Array.from(config.commands)[0] + if (someCommand) { + expect('hiddenAliases' in someCommand).toBe(true) + } + }) + }) +}) diff --git a/packages/cli-kit/src/public/node/custom-oclif-loader.ts b/packages/cli-kit/src/public/node/custom-oclif-loader.ts index 527f837eab..c57fddbc23 100644 --- a/packages/cli-kit/src/public/node/custom-oclif-loader.ts +++ b/packages/cli-kit/src/public/node/custom-oclif-loader.ts @@ -2,9 +2,15 @@ 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' import {Options} from '@oclif/core/interfaces' +interface OclifCommand { + id: string + aliases?: string[] + hiddenAliases?: string[] +} + export class ShopifyConfig extends Config { constructor(options: Options) { if (isDevelopment()) { @@ -14,9 +20,11 @@ export class ShopifyConfig extends Config { // Hydrogen CI uses `hydrogen/hydrogen` path, while local dev uses `shopify/hydrogen`. const currentPathMightBeHydrogenMonorepo = /(shopify|hydrogen)\/hydrogen/i.test(currentPath) const ignoreHydrogenMonorepo = process.env.IGNORE_HYDROGEN_MONOREPO - if (currentPathMightBeHydrogenMonorepo && !ignoreHydrogenMonorepo) { + const useHydrogenMonorepo = currentPathMightBeHydrogenMonorepo && !ignoreHydrogenMonorepo + if (useHydrogenMonorepo) { 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) @@ -25,69 +33,56 @@ export class ShopifyConfig extends Config { core: ['@shopify/cli-hydrogen'], path, } + if (useHydrogenMonorepo) { + // Skip the oclif manifest cache so external plugin commands are loaded + // from disk rather than from a potentially stale manifest. This has a + // startup performance cost, but only applies inside the Hydrogen monorepo. + options.ignoreManifest = true + } } } 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. - - 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 - } + async load(): Promise { + await super.load() - if (aCommand.pluginAlias === '@shopify/cli-hydrogen' && bCommand.pluginType === 'core') { - // If a is hydrogen and b is core sort a first - return -1 - } + if (!isDevelopment()) return - // 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 - } + // 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 b is a core plugin and a is not sort b first - if (bCommand.pluginType === 'core' && aCommand.pluginType !== 'core') { - return 1 - } + if (!externalHydrogenPlugin) return - // if a is a core plugin and b is not sort a first - if (aCommand.pluginType === 'core' && bCommand.pluginType !== 'core') { - return -1 - } + if (typeof (this as unknown as {[key: string]: unknown})._commands === 'undefined') { + throw new Error('ShopifyConfig: oclif internals changed. _commands is no longer available.') + } - // if a is a jit plugin and b is not sort b first - if (aCommand.pluginType === 'jit' && bCommand.pluginType !== 'jit') { - return 1 - } + // @ts-expect-error: _commands is private but we need to replace bundled commands + const internalCommands = this._commands as Map + if (!(internalCommands instanceof Map)) { + throw new Error('ShopifyConfig: oclif internals changed. _commands is no longer a Map.') + } - // if b is a jit plugin and a is not sort a first - if (bCommand.pluginType === 'jit' && aCommand.pluginType !== 'jit') { - return -1 + // Delete all bundled hydrogen command entries (canonical IDs, aliases, and hidden aliases) + // before reloading from the external plugin. This mirrors oclif's own insertLegacyPlugins + // pattern and ensures alias entries don't continue pointing to the bundled version. + for (const command of externalHydrogenPlugin.commands) { + if (!command.id.startsWith('hydrogen')) continue + internalCommands.delete(command.id) + const allAliases = [...(command.aliases ?? []), ...((command as OclifCommand).hiddenAliases ?? [])] + for (const alias of allAliases) { + internalCommands.delete(alias) } + } - // neither plugin is core, so do not change the order - return 0 - }) - return commandPlugins[0] + // Let oclif's own loadCommands re-insert commands with proper alias and permutation + // handling, mirroring the insertLegacyPlugins pattern used for legacy plugins. + // @ts-expect-error: loadCommands is private but handles aliases/permutations correctly + this.loadCommands(externalHydrogenPlugin) } }