Skip to content
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.

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

Show a warning when there are multiple CLI installations #4629

Merged
merged 22 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
1771fc8
Show a warning when there are multiple CLI installations
gonzaloriestra Oct 10, 2024
f435e23
Show a warning when there are multiple CLI installations
gonzaloriestra Oct 10, 2024
03b39a6
Add changeset
gonzaloriestra Oct 10, 2024
c729022
Merge branch 'multiple-installations-warning' of https://github.com/S…
gonzaloriestra Oct 10, 2024
447a25a
Remove console.log
gonzaloriestra Oct 10, 2024
75fc0e9
Merge with current global CLI warning
gonzaloriestra Oct 10, 2024
ff9de0d
Clean up
gonzaloriestra Oct 10, 2024
a74f30b
Update documentation link
gonzaloriestra Oct 11, 2024
5fd47d8
WIP
gonzaloriestra Oct 16, 2024
b6c6080
Update message to show current versions
gonzaloriestra Oct 17, 2024
54cae9c
Merge branch 'main' into multiple-installations-warning
gonzaloriestra Oct 17, 2024
cb38917
Fix tests
gonzaloriestra Oct 17, 2024
2b4fde9
Update message
gonzaloriestra Oct 17, 2024
3dc32f1
Update link message
gonzaloriestra Oct 21, 2024
a720de9
Add a final dot
gonzaloriestra Oct 21, 2024
0962a91
Merge branch 'main' into multiple-installations-warning
gonzaloriestra Oct 21, 2024
83355ad
Avoid npm command when the package.json does not include the CLI depe…
gonzaloriestra Oct 22, 2024
3c5c51b
Avoid extra command when checking the global version
gonzaloriestra Oct 22, 2024
d5d7567
Fix lint issues
gonzaloriestra Oct 22, 2024
b36e968
Merge branch 'main' into multiple-installations-warning
gonzaloriestra Oct 22, 2024
a6fae7d
Move version functions to a new file
gonzaloriestra Oct 23, 2024
e1a81c2
Run internal shopify commands with SHOPIFY_CLI_NO_ANALYTICS
gonzaloriestra Oct 23, 2024
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
6 changes: 6 additions & 0 deletions .changeset/moody-beds-sneeze.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@shopify/cli-kit': patch
'@shopify/app': patch
---

Show a warning when there are multiple CLI installations
34 changes: 21 additions & 13 deletions packages/app/src/cli/models/app/loader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,25 @@ import {
pnpmLockfile,
PackageJson,
pnpmWorkspaceFile,
localCLIVersion,
} from '@shopify/cli-kit/node/node-package-manager'
import {inTemporaryDirectory, moveFile, mkdir, mkTmpDir, rmdir, writeFile} from '@shopify/cli-kit/node/fs'
import {joinPath, dirname, cwd, normalizePath} from '@shopify/cli-kit/node/path'
import {platformAndArch} from '@shopify/cli-kit/node/os'
import {outputContent, outputToken} from '@shopify/cli-kit/node/output'
import {zod} from '@shopify/cli-kit/node/schema'
import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output'
import {currentProcessIsGlobal} from '@shopify/cli-kit/node/is-global'
import colors from '@shopify/cli-kit/node/colors'
// eslint-disable-next-line no-restricted-imports
import {resolve} from 'path'

import {globalCLIVersion, isGlobalCLIInstalled} from '@shopify/cli-kit/node/is-global'

vi.mock('../../services/local-storage.js')
vi.mock('../../services/app/config/use.js')
vi.mock('@shopify/cli-kit/node/is-global')
vi.mock('@shopify/cli-kit/node/node-package-manager', async () => ({
...((await vi.importActual('@shopify/cli-kit/node/node-package-manager')) as any),
localCLIVersion: vi.fn(),
}))

describe('load', () => {
let specifications: ExtensionSpecification[] = []
Expand Down Expand Up @@ -322,9 +326,10 @@ wrong = "property"

test('shows warning if using global CLI but app has local dependency', async () => {
// Given
vi.mocked(currentProcessIsGlobal).mockReturnValueOnce(true)
vi.mocked(isGlobalCLIInstalled).mockResolvedValue(true)
vi.mocked(globalCLIVersion).mockResolvedValue('3.68.0')
vi.mocked(localCLIVersion).mockResolvedValue('3.65.0')
const mockOutput = mockAndCaptureOutput()
mockOutput.clear()
await writeConfig(appConfiguration, {
workspaces: ['packages/*'],
name: 'my_app',
Expand All @@ -339,25 +344,28 @@ wrong = "property"
expect(mockOutput.info()).toMatchInlineSnapshot(`
"╭─ info ───────────────────────────────────────────────────────────────────────╮
│ │
You are running a global installation of Shopify CLI
Two Shopify CLI installations found – using local dependency
│ │
│ This project has Shopify CLI as a local dependency in package.json. If you │
│ prefer to use that version, run the command with your package manager │
│ (e.g. npm run shopify). │
│ A global installation (v3.68.0) and a local dependency (v3.65.0) were │
│ detected. │
│ We recommend removing the @shopify/cli and @shopify/app dependencies from │
│ your package.json, unless you want to use different versions across │
│ multiple apps. │
│ │
For more information, see Shopify CLI documentation [1] │
See Shopify CLI documentation. [1]
│ │
╰──────────────────────────────────────────────────────────────────────────────╯
[1] https://shopify.dev/docs/apps/tools/cli
[1] https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executab
le-or-local-dependency
"
`)
mockOutput.clear()
})

test('doesnt show warning if there is no local dependency', async () => {
// Given
vi.mocked(localCLIVersion).mockResolvedValue(undefined)
const mockOutput = mockAndCaptureOutput()
mockOutput.clear()
await writeConfig(appConfiguration, {
workspaces: ['packages/*'],
name: 'my_app',
Expand Down Expand Up @@ -2325,7 +2333,7 @@ wrong = "property"

// Then
expect(use).toHaveBeenCalledWith({
directory: normalizePath(resolve(tmpDir)),
directory: normalizePath(tmpDir),
shouldRenderSuccess: false,
warningContent: {
headline: "Couldn't find shopify.app.non-existent.toml",
Expand Down
32 changes: 20 additions & 12 deletions packages/app/src/cli/models/app/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
getPackageManager,
getPackageName,
usesWorkspaces as appUsesWorkspaces,
localCLIVersion,
} from '@shopify/cli-kit/node/node-package-manager'
import {resolveFramework} from '@shopify/cli-kit/node/framework'
import {hashString} from '@shopify/cli-kit/node/crypto'
Expand All @@ -48,7 +49,7 @@ import {joinWithAnd, slugify} from '@shopify/cli-kit/common/string'
import {getArrayRejectingUndefined} from '@shopify/cli-kit/common/array'
import {checkIfIgnoredInGitRepository} from '@shopify/cli-kit/node/git'
import {renderInfo} from '@shopify/cli-kit/node/ui'
import {currentProcessIsGlobal} from '@shopify/cli-kit/node/is-global'
import {currentProcessIsGlobal, globalCLIVersion} from '@shopify/cli-kit/node/is-global'

const defaultExtensionDirectory = 'extensions/*'

Expand Down Expand Up @@ -304,7 +305,7 @@ class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionS
const name = await loadAppName(directory)
const nodeDependencies = await getDependencies(packageJSONPath)
const packageManager = await getPackageManager(directory)
this.showGlobalCLIWarningIfNeeded(nodeDependencies, packageManager)
await this.showMultipleCLIWarningIfNeeded(directory)
const {webs, usedCustomLayout: usedCustomLayoutForWeb} = await this.loadWebs(
directory,
configuration.web_directories,
Expand Down Expand Up @@ -352,21 +353,28 @@ class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionS
return parseConfigurationFile(schema, filepath, this.abortOrReport.bind(this), decode)
}

private showGlobalCLIWarningIfNeeded(nodeDependencies: {[key: string]: string}, packageManager: string) {
const hasLocalCLI = nodeDependencies['@shopify/cli'] !== undefined
// Show the warning IFF:
// - The current process is global
// - The project has a local CLI
private async showMultipleCLIWarningIfNeeded(directory: string) {
// Show the warning if:
// - There is a global installation
// - The project has a local CLI dependency
// - The user didn't include the --json flag (to avoid showing the warning in scripts or CI/CD pipelines)
if (currentProcessIsGlobal() && hasLocalCLI && !sniffForJson() && !alreadyShownCLIWarning) {
// - The warning hasn't been shown yet during the current command execution

const localVersion = await localCLIVersion(directory)
const globalVersion = await globalCLIVersion()

if (localVersion && globalVersion && !sniffForJson() && !alreadyShownCLIWarning) {
const currentInstallation = currentProcessIsGlobal() ? 'global installation' : 'local dependency'

const warningContent = {
headline: 'You are running a global installation of Shopify CLI',
headline: `Two Shopify CLI installations found – using ${currentInstallation}`,
body: [
`This project has Shopify CLI as a local dependency in package.json. If you prefer to use that version, run the command with your package manager (e.g. ${packageManager} run shopify).`,
`A global installation (v${globalVersion}) and a local dependency (v${localVersion}) were detected.
We recommend removing the @shopify/cli and @shopify/app dependencies from your package.json, unless you want to use different versions across multiple apps.`,
],
link: {
label: 'For more information, see Shopify CLI documentation',
url: 'https://shopify.dev/docs/apps/tools/cli',
label: 'See Shopify CLI documentation.',
url: 'https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executable-or-local-dependency',
},
}
renderInfo(warningContent)
Expand Down
15 changes: 15 additions & 0 deletions packages/cli-kit/src/public/node/is-global.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,21 @@ export async function isGlobalCLIInstalled(): Promise<boolean> {
}
}

/**
* Returns true if the global CLI is installed.
*
* @returns `true` if the global CLI is installed.
*/
export async function globalCLIVersion(): Promise<string | undefined> {
try {
if (!(await isGlobalCLIInstalled())) return undefined
return captureOutput('shopify', ['version'])
// eslint-disable-next-line no-catch-all/no-catch-all
gonzaloriestra marked this conversation as resolved.
Show resolved Hide resolved
} catch {
return undefined
}
}

/**
* Installs the global Shopify CLI, using the provided package manager.
*
Expand Down
31 changes: 31 additions & 0 deletions packages/cli-kit/src/public/node/node-package-manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
checkForCachedNewVersion,
inferPackageManager,
PackageManager,
localCLIVersion,
} from './node-package-manager.js'
import {captureOutput, exec} from './system.js'
import {inTemporaryDirectory, mkdir, touchFile, writeFile} from './fs.js'
Expand Down Expand Up @@ -1027,3 +1028,33 @@ describe('inferPackageManager', () => {
expect(inferPackageManager(undefined, mockEnv)).toBe('npm')
})
})

describe('localCLIVersion', () => {
test('returns the version of the local CLI', async () => {
await inTemporaryDirectory(async (tmpDir) => {
// Given
vi.mocked(captureOutput).mockResolvedValueOnce(`folder@ ${tmpDir}
└── @shopify/cli@3.68.0`)

// When
const got = await localCLIVersion(tmpDir)

// Then
expect(got).toEqual('3.68.0')
})
})

test('returns undefined when the dependency is not installed', async () => {
await inTemporaryDirectory(async (tmpDir) => {
// Given
vi.mocked(captureOutput).mockResolvedValueOnce(`folder@ ${tmpDir}
└── (empty)`)

// When
const got = await localCLIVersion(tmpDir)

// Then
expect(got).toBeUndefined()
})
})
})
16 changes: 16 additions & 0 deletions packages/cli-kit/src/public/node/node-package-manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -732,3 +732,19 @@ export function inferPackageManager(optionsPackageManager: string | undefined, e

return 'npm'
}

/**
* Returns the version of the local dependency of the CLI if it's installed in the provided directory.
*
* @param directory - path of the project to look for the dependency.
* @returns the CLI version or undefined if the dependency is not installed.
*/
export async function localCLIVersion(directory: string): Promise<string | undefined> {
try {
const output = await captureOutput('npm', ['list', '@shopify/cli'], {cwd: directory})
return output.match(/@shopify\/cli@([\w.-]*)/)?.[1]
// eslint-disable-next-line no-catch-all/no-catch-all
} catch {
return undefined
}
gonzaloriestra marked this conversation as resolved.
Show resolved Hide resolved
}