cli icon indicating copy to clipboard operation
cli copied to clipboard

Custom network settings for theme commands

Open EvilGenius13 opened this issue 8 months ago β€’ 6 comments

Fixes https://github.com/Shopify/cli/issues/5617

WHAT is this pull request doing?

With the implementation of some new request behaviour, some theme commands were failing if they took longer than 15 seconds (the new default timeout). This can affect devs with larger themes and/or slower internet.

Enhancements to API Request Behavior:

  • Added RequestModeInput and RequestBehaviour types to packages/cli-kit/src/public/node/http.ts to define customizable network request behaviors.
  • Updated adminRequestDoc and performGraphQLRequest functions to accept an optional requestBehaviour parameter for configuring request behavior. [1] [2]
  • Introduced a constant THEME_API_NETWORK_BEHAVIOUR in packages/cli-kit/src/public/node/themes/api.ts to specify default network behavior for theme-related API calls.

These changes improve the robustness and configurability of API requests, particularly for theme-related operations.

How to test your changes?

Option 1:

  • Pull down the branch
  • Build the branch
  • Download software that can manipulate your internet speed and slow it down to the point a theme command like theme push takes over 15 seconds to complete. You shouldn't see a timeout error until 45 seconds (original 15 seconds)

Option 2:

  • Pull down the branch
  • in /src/public/node/themes/api.ts, in the constant THEME_API_NETWORK_BEHAVIOUR, change timeoutMs and maxRetryTimeMs to 1000 (1 second)
  • Build the branch
  • Run theme push and it should fail almost instantly. Now we know the changes are affecting the right area.

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)

EvilGenius13 avatar Apr 25 '25 20:04 EvilGenius13

Coverage report

St.:grey_question:
Category Percentage Covered / Total
🟑 Statements
76.63% (+0.01% πŸ”Ό)
9586/12509
🟑 Branches
71.88% (+0.02% πŸ”Ό)
4723/6571
🟑 Functions 76.56% 2485/3246
🟑 Lines
77.14% (+0.01% πŸ”Ό)
9059/11743

Test suite run success

2251 tests passing in 981 suites.

Report generated by πŸ§ͺjest coverage report action from 842200406cd8338d601201f6796623d3ac2a956c

github-actions[bot] avatar Apr 28 '25 19:04 github-actions[bot]

We detected some changes at packages/*/src and there are no updates in the .changeset. If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

github-actions[bot] avatar Apr 28 '25 20:04 github-actions[bot]

Can we loop in @shauns to this for visibility + an extra set of eyes?

jamesmengo avatar Apr 28 '25 22:04 jamesmengo

Thanks for chiming in @shauns & @isaacroldan! I'll take a look at the other PR, do some edits and circle back.

EvilGenius13 avatar Apr 29 '25 17:04 EvilGenius13

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/api/admin.d.ts
@@ -1,5 +1,6 @@
 import { GraphQLResponseOptions, GraphQLVariables } from './graphql.js';
 import { AdminSession } from '../session.js';
+import { RequestModeInput } from '../http.js';
 import { Variables } from 'graphql-request';
 import { TypedDocumentNode } from '@graphql-typed-document-node/core';
 /**
@@ -11,17 +12,27 @@ import { TypedDocumentNode } from '@graphql-typed-document-node/core';
  * @returns The response of the query of generic type <T>.
  */
 export declare function adminRequest<T>(query: string, session: AdminSession, variables?: GraphQLVariables): Promise<T>;
+export interface AdminRequestOptions<TResult, TVariables extends Variables> {
+    /** GraphQL query to execute. */
+    query: TypedDocumentNode<TResult, TVariables>;
+    /** Shopify admin session including token and Store FQDN. */
+    session: AdminSession;
+    /** GraphQL variables to pass to the query. */
+    variables?: TVariables;
+    /** API version. */
+    version?: string;
+    /** Control how API responses will be handled. */
+    responseOptions?: GraphQLResponseOptions<TResult>;
+    /** Custom request behaviour for retries and timeouts. */
+    requestBehaviour?: RequestModeInput;
+}
 /**
  * Executes a GraphQL query against the Admin API. Uses typed documents.
  *
- * @param query - GraphQL query to execute.
- * @param session - Shopify admin session including token and Store FQDN.
- * @param variables - GraphQL variables to pass to the query.
- * @param version - API version.
- * @param responseOptions - Control how API responses will be handled.
+ * @param options - Admin request options.
  * @returns The response of the query of generic type <TResult>.
  */
-export declare function adminRequestDoc<TResult, TVariables extends Variables>(query: TypedDocumentNode<TResult, TVariables>, session: AdminSession, variables?: TVariables, version?: string, responseOptions?: GraphQLResponseOptions<TResult>): Promise<TResult>;
+export declare function adminRequestDoc<TResult, TVariables extends Variables>(options: AdminRequestOptions<TResult, TVariables>): Promise<TResult>;
 /**
  * GraphQL query to retrieve all supported API versions.
  *

packages/cli-kit/dist/public/node/api/graphql.d.ts
@@ -32,6 +32,7 @@ export type GraphQLRequestOptions<T> = GraphQLRequestBaseOptions<T> & {
     query: RequestDocument;
     variables?: Variables;
     unauthorizedHandler?: () => Promise<void>;
+    requestBehaviour?: RequestModeInput;
 };
 export type GraphQLRequestDocOptions<TResult, TVariables> = GraphQLRequestBaseOptions<TResult> & {
     query: TypedDocumentNode<TResult, TVariables> | TypedDocumentNode<TResult, Exact<{
@@ -39,6 +40,7 @@ export type GraphQLRequestDocOptions<TResult, TVariables> = GraphQLRequestBaseOp
     }>>;
     variables?: TVariables;
     unauthorizedHandler?: () => Promise<void>;
+    requestBehaviour?: RequestModeInput;
 };
 export interface GraphQLResponseOptions<T> {
     handleErrors?: boolean;

github-actions[bot] avatar May 02 '25 15:05 github-actions[bot]

@shauns or @isaacroldan, could you please take a look at this when you have a moment?

karreiro avatar May 09 '25 12:05 karreiro