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

Skip notifications when using --json #4930

Merged
merged 11 commits into from
Nov 27, 2024

Conversation

gonzaloriestra
Copy link
Contributor

@gonzaloriestra gonzaloriestra commented Nov 27, 2024

WHY are these changes introduced?

Fixes #4925

WHAT is this pull request doing?

  • Prevents notifications from being shown when the --json or -j flags are used, or the SHOPIFY_FLAG_JSON environment variable
  • Unifies the usage of the --json flag across commands. Some where missing the shortcut -j and the descriptions were different.
  • Applies the same logic to the showMultipleCLIWarningIfNeeded, where we were only checking for --json

How to test your changes?

  • pnpm i -g @shopify/[email protected]
  • SHOPIFY_CLI_NOTIFICATIONS_URL=https://shopify.link/MNbn shopify theme list
  • SHOPIFY_CLI_NOTIFICATIONS_URL=https://shopify.link/MNbn shopify theme list --json
  • SHOPIFY_CLI_NOTIFICATIONS_URL=https://shopify.link/MNbn shopify theme list -j
  • SHOPIFY_FLAG_JSON=1 SHOPIFY_CLI_NOTIFICATIONS_URL=https://shopify.link/MNbn shopify theme list

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

@gonzaloriestra gonzaloriestra changed the title Skip notifications with json flag Skip notifications when using --json Nov 27, 2024
Copy link
Contributor

github-actions bot commented Nov 27, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.29% (+0.03% 🔼)
8633/11466
🟡 Branches
70.64% (-0.04% 🔻)
4208/5957
🟡 Functions
74.92% (+0.09% 🔼)
2267/3026
🟡 Lines
75.85% (+0.03% 🔼)
8165/10764
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
93.42% (-1.32% 🔻)
87.88% (-3.03% 🔻)
90.48% 98.53%
🟢
... / ConcurrentOutput.tsx
98.36% (-1.64% 🔻)
88% (-4% 🔻)
100%
98.33% (-1.67% 🔻)
🟢
... / path.ts
79.31% (+2.64% 🔼)
65% (-7.22% 🔻)
75%
80.77% (+2.99% 🔼)

Test suite run success

1954 tests passing in 888 suites.

Report generated by 🧪jest coverage report action from 9ed8458

@gonzaloriestra
Copy link
Contributor Author

/snapit

Copy link
Contributor

🫰✨ 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.

@gonzaloriestra gonzaloriestra marked this pull request as ready for review November 27, 2024 12:31
@gonzaloriestra gonzaloriestra requested review from a team as code owners November 27, 2024 12:31
packages/cli-kit/src/public/node/cli.ts Outdated Show resolved Hide resolved
Copy link
Contributor

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/cli.d.ts
@@ -33,6 +33,9 @@ export declare const globalFlags: {
     'no-color': import("@oclif/core/lib/interfaces/parser.js").BooleanFlag<boolean>;
     verbose: import("@oclif/core/lib/interfaces/parser.js").BooleanFlag<boolean>;
 };
+export declare const jsonFlag: {
+    json: import("@oclif/core/lib/interfaces/parser.js").BooleanFlag<boolean>;
+};
 /**
  * Clear the CLI cache, used to store some API responses and handle notifications status
  */
packages/cli-kit/dist/public/node/environment.d.ts
@@ -43,4 +43,11 @@ export declare function getIdentityTokenInformation(): {
     accessToken: string;
     refreshToken: string;
     userId: string;
-} | undefined;
\ No newline at end of file
+} | undefined;
+/**
+ * Checks if the JSON output is enabled via flag (--json or -j) or environment variable (SHOPIFY_FLAG_JSON).
+ *
+ * @param environment - Process environment variables.
+ * @returns True if the JSON output is enabled, false otherwise.
+ */
+export declare function jsonOutputEnabled(environment?: NodeJS.ProcessEnv): boolean;
\ No newline at end of file
packages/cli-kit/dist/public/node/path.d.ts
 /// <reference types="node" resolution-mode="require"/>
 import type { URL } from 'url';
 /**
  * Joins a list of paths together.
  *
  * @param paths - Paths to join.
  * @returns Joined path.
  */
 export declare function joinPath(...paths: string[]): string;
 /**
  * Normalizes a path.
  *
  * @param path - Path to normalize.
  * @returns Normalized path.
  */
 export declare function normalizePath(path: string): string;
 /**
  * Resolves a list of paths together.
  *
  * @param paths - Paths to resolve.
  * @returns Resolved path.
  */
 export declare function resolvePath(...paths: string[]): string;
 /**
  * Returns the relative path from one path to another.
  *
  * @param from - Path to resolve from.
  * @param to - Path to resolve to.
  * @returns Relative path.
  */
 export declare function relativePath(from: string, to: string): string;
 /**
  * Returns whether the path is absolute.
  *
  * @param path - Path to check.
  * @returns Whether the path is absolute.
  */
 export declare function isAbsolutePath(path: string): boolean;
 /**
  * Returns the directory name of a path.
  *
  * @param path - Path to get the directory name of.
  * @returns Directory name.
  */
 export declare function dirname(path: string): string;
 /**
  * Returns the base name of a path.
  *
  * @param path - Path to get the base name of.
  * @param ext - Optional extension to remove from the result.
  * @returns Base name.
  */
 export declare function basename(path: string, ext?: string): string;
 /**
  * Returns the extension of the path.
  *
  * @param path - Path to get the extension of.
  * @returns Extension.
  */
 export declare function extname(path: string): string;
 /**
  * Given an absolute filesystem path, it makes it relative to
  * the current working directory. This is useful when logging paths
  * to allow the users to click on the file and let the OS open it
  * in the editor of choice.
  *
  * @param path - Path to relativize.
  * @param dir - Current working directory.
  * @returns Relativized path.
  */
 export declare function relativizePath(path: string, dir?: string): string;
 /**
  * Given 2 paths, it returns whether the second path is a subpath of the first path.
  *
  * @param mainPath - The main path.
  * @param subpath - The subpath.
  * @returns Whether the subpath is a subpath of the main path.
  */
 export declare function isSubpath(mainPath: string, subpath: string): boolean;
 /**
  * Given a module's import.meta.url it returns the directory containing the module.
  *
  * @param moduleURL - The value of import.meta.url in the context of the caller module.
  * @returns The path to the directory containing the caller module.
  */
 export declare function moduleDirectory(moduleURL: string | URL): string;
 /**
  * When running a script using `npm run`, something interesting happens. If the current
  * folder does not have a `package.json` or a `node_modules` folder, npm will traverse
  * the directory tree upwards until it finds one. Then it will run the script and set
  * `process.cwd()` to that folder, while the actual path is stored in the INIT_CWD
  * environment variable (see here: https://docs.npmjs.com/cli/v9/commands/npm-run-script#description).
  *
  * @returns The path to the current working directory.
  */
 export declare function cwd(): string;
 /**
  * Tries to get the value of the `--path` argument, if provided.
  *
  * @param argv - The arguments to search for the `--path` argument.
  * @returns The value of the `--path` argument, if provided.
  */
 export declare function sniffForPath(argv?: string[]): string | undefined;
 /**
- * Returns whether the `--json` flag is present in the arguments.
+ * Returns whether the `--json` or `-j` flags are present in the arguments.
  *
- * @param argv - The arguments to search for the `--json` flag.
- * @returns Whether the `--json` flag is present in the arguments.
+ * @param argv - The arguments to search for the `--json` and `-j` flags.
+ * @returns Whether the `--json` or `-j` flag is present in the arguments.
  */
 export declare function sniffForJson(argv?: string[]): boolean;
packages/cli-kit/dist/private/node/constants.d.ts
@@ -30,6 +30,7 @@ export declare const environmentVariables: {
     refreshToken: string;
     otelURL: string;
     themeKitAccessDomain: string;
+    json: string;
 };
 export declare const defaultThemeKitAccessDomain = "theme-kit-access.shopifyapps.com";
 export declare const systemEnvironmentVariables: {

@gonzaloriestra gonzaloriestra added this pull request to the merge queue Nov 27, 2024
Merged via the queue into main with commit aecf7fe Nov 27, 2024
27 checks passed
@gonzaloriestra gonzaloriestra deleted the skip-notifications-with-json-flag branch November 27, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: New notification system breaks json commands output
2 participants