Skip to content

Commit

Permalink
Improve multiple CLI installations warning performance (#4999)
Browse files Browse the repository at this point in the history
### WHY are these changes introduced?

Checking for multiple installed CLIs is very expensive.

### WHAT is this pull request doing?

Wraps the `showMultipleCLIWarningIfNeeded` function call with `runAtMinimumInterval` to ensure the warning is only checked once every 24 hours.

### How to test your changes?

1. Having a local CLI and a global CLI
2. Run `shopify app info`, you should see a warning
3. Run it again: no warning -> command runs faster 

### Measuring impact

How do we know this change was effective? Please choose one:

- [x] n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

### Checklist

- [x] I've considered possible cross-platform impacts (Mac, Linux, Windows)
- [x] I've considered possible [documentation](https://shopify.dev) changes
  • Loading branch information
isaacroldan authored Nov 29, 2024
2 parents 8d21357 + 5b0d610 commit b70d526
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 21 deletions.
4 changes: 2 additions & 2 deletions packages/app/src/cli/models/app/loader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output'
import colors from '@shopify/cli-kit/node/colors'

import {globalCLIVersion, localCLIVersion} from '@shopify/cli-kit/node/version'
import {CLI_KIT_VERSION} from '@shopify/cli-kit/common/version'

vi.mock('../../services/local-storage.js')
vi.mock('../../services/app/config/use.js')
Expand Down Expand Up @@ -329,7 +330,6 @@ wrong = "property"
test('shows warning if using global CLI but app has local dependency', async () => {
// Given
vi.mocked(globalCLIVersion).mockResolvedValue('3.68.0')
vi.mocked(localCLIVersion).mockResolvedValue('3.65.0')
const mockOutput = mockAndCaptureOutput()
await writeConfig(appConfiguration, {
workspaces: ['packages/*'],
Expand All @@ -347,7 +347,7 @@ wrong = "property"
│ │
│ Two Shopify CLI installations found – using local dependency │
│ │
│ A global installation (v3.68.0) and a local dependency (v3.65.0) were │
│ A global installation (v3.68.0) and a local dependency (v${CLI_KIT_VERSION}) were │
│ detected. │
│ We recommend removing the @shopify/cli and @shopify/app dependencies from │
│ your package.json, unless you want to use different versions across │
Expand Down
50 changes: 31 additions & 19 deletions packages/app/src/cli/models/app/validation/multi-cli-warning.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,30 +2,42 @@ import {renderInfo} from '@shopify/cli-kit/node/ui'
import {currentProcessIsGlobal} from '@shopify/cli-kit/node/is-global'
import {globalCLIVersion, localCLIVersion} from '@shopify/cli-kit/node/version'
import {jsonOutputEnabled} from '@shopify/cli-kit/node/environment'
import {CLI_KIT_VERSION} from '@shopify/cli-kit/common/version'

/**
* Shows a warning if there are two Shopify CLI installations found (global and local).
* Won't show anything if the user included the --json flag
*
* @param directory - The directory of the project.
* @param dependencies - The dependencies of the project.
*/
export async function showMultipleCLIWarningIfNeeded(directory: string, dependencies: {[key: string]: 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 (!dependencies['@shopify/cli'] || jsonOutputEnabled()) return

const localVersion = dependencies['@shopify/cli'] && (await localCLIVersion(directory))
const globalVersion = await globalCLIVersion()
const isGlobal = currentProcessIsGlobal()

if (localVersion && globalVersion && !jsonOutputEnabled()) {
const currentInstallation = currentProcessIsGlobal() ? 'global installation' : 'local dependency'
// If running globally, use the current CLI version, otherwise fetch the global CLI version
// Exit early if we can't get the global version
const globalVersion = isGlobal ? CLI_KIT_VERSION : await globalCLIVersion()
if (!globalVersion) return

const warningContent = {
headline: `Two Shopify CLI installations found – using ${currentInstallation}`,
body: [
`A global installation (v${globalVersion}) and a local dependency (v${localVersion}) were detected.
// If running globally, fetch the local version from npm list, otherwise use current CLI version
// Exit early if we can't get the local version
const localVersion = isGlobal ? await localCLIVersion(directory) : CLI_KIT_VERSION
if (!localVersion) return

const currentInstallation = isGlobal ? 'global installation' : 'local dependency'

const warningContent = {
headline: `Two Shopify CLI installations found – using ${currentInstallation}`,
body: [
`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: 'See Shopify CLI documentation.',
url: 'https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executable-or-local-dependency',
},
}
renderInfo(warningContent)
],
link: {
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)
}

0 comments on commit b70d526

Please sign in to comment.