Show a warning when there are multiple CLI installations #4629

gonzaloriestra · 2024-10-10T09:06:04Z

WHY are these changes introduced?

The CLI can be installed both locally (as a project dependency) and globally at the same time, and it can be confusing for the users, who could be running an unexpected version.

We already had a warning when you run the global CLI having a local installation. But not in the opposite scenario: when you run the CLI locally having a global installation.

WHAT is this pull request doing?

Shows the warning when we detect multiple installations, no matter which one you are running:

How to test your changes?

Without a global installation:

Add the local dependency to a project: npm i @shopify/[email protected]
npm run shopify app info

Install the global dependency: npm i -g @shopify/[email protected]

shopify app info
npm run shopify app info

Measuring impact

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

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
Existing analytics will cater for this addition
PR includes analytics changes to measure impact

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes

github-actions · 2024-10-10T09:06:25Z

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

Themes: @shopify/advanced-edits
UI extensions: @shopify/ui-extensions-cli
- Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
Hydrogen: @shopify/hydrogen
Other: @shopify/app-inner-loop

…hopify/cli into multiple-installations-warning

github-actions · 2024-10-10T09:13:05Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	72.57% (-0.02% 🔻)	8361/11521
🟡	Branches	69.12% (-0.01% 🔻)	4108/5943
🟡	Functions	71.88% (-0.01% 🔻)	2186/3041
🟡	Lines	72.88% (-0.01% 🔻)	7905/10847

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / loader.ts	93.09% (+0.04% 🔼)	83.15% (-0.38% 🔻)	97.03%	94% (+0.03% 🔼)
🟡	`...` / is-global.ts	69.44% (-11.2% 🔻)	64.29% (-4.95% 🔻)	66.67% (-13.33% 🔻)	75.86% (-12.14% 🔻)
🟢	`...` / node-package-manager.ts	85.95% (-0.24% 🔻)	84.44%	86.49% (+0.38% 🔼)	85.79% (-0.24% 🔻)

Test suite run success

1905 tests passing in 866 suites.

Report generated by 🧪jest coverage report action from 2b4fde9

isaacroldan · 2024-10-11T11:32:05Z

packages/app/src/cli/models/app/loader.ts

+ 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.`],


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

And maybe link to this doc: https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executable-or-local-dependency
?

@jessehoek could you review this new message? 🙏

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?

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?

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.

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?

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?

gonzaloriestra · 2024-10-17T11:05:23Z

/snapit

github-actions · 2024-10-17T11:07:04Z

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

Some seemingly private modules might be re-exported through public modules.
If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/node/is-global.d.ts

@@ -12,6 +12,12 @@ export declare function currentProcessIsGlobal(argv?: string[]): boolean;
  * @returns  if the global CLI is installed.
  */
 export declare function isGlobalCLIInstalled(): Promise<boolean>;
+/**
+ * Returns true if the global CLI is installed.
+ *
+ * @returns  if the global CLI is installed.
+ */
+export declare function globalCLIVersion(): Promise<string | undefined>;
 /**
  * Installs the global Shopify CLI, using the provided package manager.
  *

packages/cli-kit/dist/public/node/node-package-manager.d.ts

@@ -306,4 +306,11 @@ export declare function writePackageJSON(directory: string, packageJSON: Package
  * @returns The inferred package manager as a PackageManager type.
  */
 export declare function inferPackageManager(optionsPackageManager: string | undefined, env?: NodeJS.ProcessEnv): PackageManager;
+/**
+ * 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 declare function localCLIVersion(directory: string): Promise<string | undefined>;
 export {};
\ No newline at end of file

github-actions · 2024-10-17T11:07:27Z

🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm.

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/[email protected]

After installing, validate the version by running just shopify in your terminal
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

Show a warning when there are multiple CLI installations

1771fc8

gonzaloriestra added 3 commits October 10, 2024 11:07

Show a warning when there are multiple CLI installations

f435e23

Add changeset

03b39a6

Merge branch 'multiple-installations-warning' of https://github.com/S…

c729022

…hopify/cli into multiple-installations-warning

Remove console.log

447a25a

gonzaloriestra marked this pull request as ready for review October 10, 2024 09:20

gonzaloriestra requested a review from a team as a code owner October 10, 2024 09:20

gonzaloriestra requested review from jamieguerrero and jantnovi and removed request for a team October 10, 2024 09:20

gonzaloriestra added 2 commits October 10, 2024 11:51

Merge with current global CLI warning

75fc0e9

Clean up

ff9de0d

This comment was marked as outdated.

Sign in to view

isaacroldan reviewed Oct 11, 2024

View reviewed changes

Update documentation link

a74f30b

gonzaloriestra force-pushed the multiple-installations-warning branch from 48728c4 to a74f30b Compare October 11, 2024 11:56

gonzaloriestra added 2 commits October 16, 2024 10:40

WIP

5fd47d8

Update message to show current versions

b6c6080

This comment was marked as outdated.

Sign in to view

gonzaloriestra added 3 commits October 17, 2024 12:26

Merge branch 'main' into multiple-installations-warning

54cae9c

Fix tests

cb38917

Update message

2b4fde9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a warning when there are multiple CLI installations #4629

Show a warning when there are multiple CLI installations #4629

gonzaloriestra commented Oct 10, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024

github-actions bot commented Oct 10, 2024 •

edited

Loading

This comment was marked as outdated.

This comment was marked as outdated.

isaacroldan Oct 11, 2024

isaacroldan Oct 11, 2024

gonzaloriestra Oct 11, 2024

jessehoek Oct 11, 2024 •

edited

Loading

gonzaloriestra Oct 11, 2024

jessehoek Oct 11, 2024 •

edited

Loading

gonzaloriestra Oct 17, 2024

jessehoek Oct 18, 2024

This comment was marked as outdated.

This comment was marked as outdated.

gonzaloriestra commented Oct 17, 2024

github-actions bot commented Oct 17, 2024

github-actions bot commented Oct 17, 2024

		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.`],

Show a warning when there are multiple CLI installations #4629

Are you sure you want to change the base?

Show a warning when there are multiple CLI installations #4629

Conversation

gonzaloriestra commented Oct 10, 2024 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Measuring impact

Checklist

github-actions bot commented Oct 10, 2024

github-actions bot commented Oct 10, 2024 • edited Loading

Coverage report

Test suite run success

This comment was marked as outdated.

This comment was marked as outdated.

isaacroldan Oct 11, 2024

Choose a reason for hiding this comment

isaacroldan Oct 11, 2024

Choose a reason for hiding this comment

gonzaloriestra Oct 11, 2024

Choose a reason for hiding this comment

jessehoek Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

gonzaloriestra Oct 11, 2024

Choose a reason for hiding this comment

jessehoek Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

gonzaloriestra Oct 17, 2024

Choose a reason for hiding this comment

jessehoek Oct 18, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

gonzaloriestra commented Oct 17, 2024

github-actions bot commented Oct 17, 2024

Differences in type declarations

New type declarations

Existing type declarations

github-actions bot commented Oct 17, 2024

gonzaloriestra commented Oct 10, 2024 •

edited

Loading

github-actions bot commented Oct 10, 2024 •

edited

Loading

jessehoek Oct 11, 2024 •

edited

Loading

jessehoek Oct 11, 2024 •

edited

Loading