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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
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
17 changes: 8 additions & 9 deletions packages/app/src/cli/models/app/loader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,9 @@ 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 {isGlobalCLIInstalled} from '@shopify/cli-kit/node/is-global'

vi.mock('../../services/local-storage.js')
vi.mock('../../services/app/config/use.js')
Expand Down Expand Up @@ -322,7 +321,7 @@ 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)
const mockOutput = mockAndCaptureOutput()
mockOutput.clear()
await writeConfig(appConfiguration, {
Expand All @@ -339,11 +338,11 @@ wrong = "property"
expect(mockOutput.info()).toMatchInlineSnapshot(`
"╭─ info ───────────────────────────────────────────────────────────────────────╮
│ │
│ You are running a global installation of Shopify CLI │
│ There are two installations of the CLI: global and as a dependency of the │
│ project. │
│ │
│ 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). │
│ We recommend to remove the @shopify/cli and @shopify/app dependencies from │
│ your package.json. │
│ │
│ For more information, see Shopify CLI documentation [1] │
│ │
Expand Down Expand Up @@ -2325,7 +2324,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
28 changes: 16 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,
isLocalCLIInstalled,
} 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 {isGlobalCLIInstalled} 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,18 +353,21 @@ 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
if (
(await isGlobalCLIInstalled()) &&
(await isLocalCLIInstalled(directory)) &&
!sniffForJson() &&
!alreadyShownCLIWarning
) {
const warningContent = {
headline: 'You are running a global installation of Shopify CLI',
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).`,
],
headline: 'There are two installations of the CLI: global and as a dependency of the project.',
body: [`We recommend to remove the @shopify/cli and @shopify/app dependencies from your package.json.`],
Copy link
Contributor

Choose a reason for hiding this comment

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

PR looks good, we might want to check this copy with product though

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jessehoek could you review this new message? 🙏

Copy link
Contributor

@jessehoek jessehoek Oct 11, 2024

Choose a reason for hiding this comment

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

Sure thing! Can we use the headline to let the user know which one (and which version) will be used for the current command?
Eg

In the local case:

Two Shopify CLI installations found – using local dependency: v3.50.1
A global installation (v3.68.1) and a local dependency (v3.50.1) were detected. We recommend removing the `@shopify/cli` and `@shopify/app` dependencies from your package.json.

In the global case:

Two Shopify CLI installations found – using global installation: v3.68.1
A global installation (v3.68.1) and a local dependency (v3.50.1) were detected. We recommend removing the `@shopify/cli` and `@shopify/app` dependencies from your package.json.

I think it's helpful to confirm which one is being used, and which version that is. Thoughts? Also, do we recommend removing the Global CLI if you are using the local one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! That makes total sense, and showing the versions will be very useful, because they are usually confused about what they are using.

About the recommendation, I think we prefer them to use the global CLI. And I guess most of the times they are using it by mistake. If they prefer to keep using that, they can check the documentation to uninstall the global one or simply ignore the warning.

One more doubt: is it ok to show this as an info banner? Or maybe better as a warning?

Copy link
Contributor

@jessehoek jessehoek Oct 11, 2024

Choose a reason for hiding this comment

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

Ah, I might suggest a slight rewording then, so that it doesn't feel like a dev is doing something wrong if they really want to use the local dependency approach. And I think info works well, since there's not anything we know is dangerous or wrong.

In the local case:

Two Shopify CLI installations found – using local dependency: v3.50.1
A global installation (v3.68.1) and a local dependency (v3.50.1) 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.

In the global case:

Two Shopify CLI installations found – using global installation: v3.68.1
A global installation (v3.68.1) and a local dependency (v3.50.1) 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that looks much better. I've updated the code and the screenshot in the description. I've just changed two details:

  • Removed the version from the title, because it's already in the next line
  • Separated the content in two lines

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good! Small thing - For the very last line, is it possible to only make the "Shopify CLI documentation" text the link, instead of the whole sentence?

link: {
label: 'For more information, see Shopify CLI documentation',
url: 'https://shopify.dev/docs/apps/tools/cli',
Expand Down
17 changes: 17 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,20 @@ export function inferPackageManager(optionsPackageManager: string | undefined, e

return 'npm'
}

/**
* Returns true if the CLI is installed as a local dependency.
*
* @param directory - Current directory to look for the package.json file.
* @returns `true` if the CLI is installed locally.
*/
export async function isLocalCLIInstalled(directory: string): Promise<boolean> {
try {
const packageJSONPath = joinPath(directory, 'package.json')
const dependencies = await getDependencies(packageJSONPath)
return dependencies['@shopify/cli'] !== undefined
// eslint-disable-next-line no-catch-all/no-catch-all
} catch {
return false
}
}
Loading