Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Cache some queries for 6 hours #5345

Open
wants to merge 1 commit into
base: graphql-cache
Choose a base branch
from

Conversation

isaacroldan
Copy link
Contributor

@isaacroldan isaacroldan commented Feb 3, 2025

WHY are these changes introduced?

Start using the new cache system for some queries that are run frequently and where the output doesn't change that often.

WHAT is this pull request doing?

Adds caching with a 6-hour TTL to several GraphQL queries:

  • User information retrieval
  • Organization lookup
  • App specifications fetching

How to test your changes?

  1. Make multiple requests to fetch user info, organizations, or specifications
  2. Verify that subsequent requests within the 6-hour window use cached data
  3. Confirm that cache invalidates after the TTL expires (you can find the cache file under /Users/<YOUR_USER>/Library/Preferences/shopify-cli-kit-nodejs/config.json and modify the timestamp to force expiration)

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

Copy link
Contributor Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@isaacroldan isaacroldan marked this pull request as ready for review February 3, 2025 16:26
@isaacroldan isaacroldan requested a review from a team as a code owner February 3, 2025 16:26
Copy link
Contributor

github-actions bot commented Feb 3, 2025

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.

Copy link
Contributor

github-actions bot commented Feb 3, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.5% (+0.07% 🔼)
9032/11963
🟡 Branches
70.77% (+0.18% 🔼)
4411/6233
🟡 Functions
75.29% (+0.03% 🔼)
2367/3144
🟡 Lines
76.03% (+0.08% 🔼)
8534/11225

Test suite run success

2040 tests passing in 911 suites.

Report generated by 🧪jest coverage report action from de4ea1c

Copy link
Contributor

@shauns shauns left a comment

Choose a reason for hiding this comment

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

The lack of tests for this class I think is getting to be untenable, I think its gone beyond connecting straight to the APIs. Maybe its time to improve this.

Copy link
Contributor

shauns commented Feb 3, 2025

/snapit

Copy link
Contributor

github-actions bot commented Feb 3, 2025

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

Test the snapshot by intalling your package globally:

pnpm i -g @shopify/cli@0.0.0-snapshot-20250203173903

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.

@isaacroldan isaacroldan force-pushed the 02-03-cache_some_queries_for_6_hours branch from eeb8b2a to de4ea1c Compare February 4, 2025 09:59
@@ -174,7 +174,10 @@ export class AppManagementClient implements DeveloperPlatformClient {
const tokenResult = await ensureAuthenticatedAppManagementAndBusinessPlatform()
const {appManagementToken, businessPlatformToken, userId} = tokenResult

const userInfoResult = await businessPlatformRequestDoc(UserInfo, businessPlatformToken)
const userInfoResult = await businessPlatformRequestDoc(UserInfo, businessPlatformToken, undefined, {
Copy link
Contributor

Choose a reason for hiding this comment

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

An example of my comment from the other PR in the stack. Is every caller of session OK with a 6hr TTL? How can we know this without having more context of the caller and how they use the values on the session? Will every future caller of session be ok with a 6hr TTL?

(should the TTL be passed into this method too?)

@isaacroldan isaacroldan force-pushed the 02-03-cache_some_queries_for_6_hours branch from de4ea1c to 0070e0d Compare February 7, 2025 10:36
@isaacroldan isaacroldan force-pushed the 02-03-cache_some_queries_for_6_hours branch from 0070e0d to d0a4778 Compare February 7, 2025 10:59
@isaacroldan isaacroldan force-pushed the 02-03-cache_some_queries_for_6_hours branch from d0a4778 to 47bc8ab Compare February 7, 2025 15:23
Copy link
Contributor

github-actions bot commented Feb 7, 2025

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
@@ -31,6 +31,7 @@ export declare function runCreateCLI(options: RunCLIOptions, launchCLI?: (option
 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>;
+    'no-cache': import("@oclif/core/lib/interfaces/parser.js").BooleanFlag<boolean>;
 };
 export declare const jsonFlag: {
     json: import("@oclif/core/lib/interfaces/parser.js").BooleanFlag<boolean>;
packages/cli-kit/dist/private/node/conf-store.d.ts
@@ -7,15 +7,17 @@ export type IntrospectionUrlKey = ;
 export type PackageVersionKey = ;
 export type NotificationsKey = ;
 export type NotificationKey = ;
+export type GraphQLRequestKey = ;
 type MostRecentOccurrenceKey = ;
 type RateLimitKey = ;
-type ExportedKey = IntrospectionUrlKey | PackageVersionKey | NotificationsKey | NotificationKey;
+type ExportedKey = IntrospectionUrlKey | PackageVersionKey | NotificationsKey | NotificationKey | GraphQLRequestKey;
 interface Cache {
     [introspectionUrlKey: IntrospectionUrlKey]: CacheValue<string>;
     [packageVersionKey: PackageVersionKey]: CacheValue<string>;
     [notifications: NotificationsKey]: CacheValue<string>;
     [notification: NotificationKey]: CacheValue<string>;
-    [MostRecentOccurrenceKey: MostRecentOccurrenceKey]: CacheValue<boolean>;
+    [graphQLRequestKey: GraphQLRequestKey]: CacheValue<string>;
+    [mostRecentOccurrenceKey: MostRecentOccurrenceKey]: CacheValue<boolean>;
     [rateLimitKey: RateLimitKey]: CacheValue<number[]>;
 }
 export interface ConfSchema {
packages/cli-kit/dist/public/node/api/app-management.d.ts
@@ -1,4 +1,4 @@
-import { GraphQLResponse } from './graphql.js';
+import { CacheOptions, GraphQLResponse } from './graphql.js';
 import { TypedDocumentNode } from '@graphql-typed-document-node/core';
 import { Variables } from 'graphql-request';
 /**
@@ -8,9 +8,10 @@ import { Variables } from 'graphql-request';
  * @param query - GraphQL query to execute.
  * @param token - Partners token.
  * @param variables - GraphQL variables to pass to the query.
+ * @param cacheOptions - Cache options for the request. If not present, the request will not be cached.
  * @returns The response of the query of generic type <T>.
  */
-export declare function appManagementRequestDoc<TResult, TVariables extends Variables>(orgId: string, query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables): Promise<TResult>;
+export declare function appManagementRequestDoc<TResult, TVariables extends Variables>(orgId: string, query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables, cacheOptions?: CacheOptions): Promise<TResult>;
 /**
  * Sets the next deprecation date from [GraphQL response extensions](https://www.apollographql.com/docs/resources/graphql-glossary/#extensions)
  * if  objects contain a  (ISO 8601-formatted string).
packages/cli-kit/dist/public/node/api/business-platform.d.ts
@@ -1,4 +1,4 @@
-import { Exact, GraphQLVariables } from './graphql.js';
+import { CacheOptions, Exact, GraphQLVariables } from './graphql.js';
 import { TypedDocumentNode } from '@graphql-typed-document-node/core';
 import { Variables } from 'graphql-request';
 /**
@@ -7,18 +7,20 @@ import { Variables } from 'graphql-request';
  * @param query - GraphQL query to execute.
  * @param token - Business Platform token.
  * @param variables - GraphQL variables to pass to the query.
+ * @param cacheOptions - Cache options for the request. If not present, the request will not be cached.
  * @returns The response of the query of generic type <T>.
  */
-export declare function businessPlatformRequest<T>(query: string, token: string, variables?: GraphQLVariables): Promise<T>;
+export declare function businessPlatformRequest<T>(query: string, token: string, variables?: GraphQLVariables, cacheOptions?: CacheOptions): Promise<T>;
 /**
  * Executes a GraphQL query against the Business Platform Destinations API. Uses typed documents.
  *
  * @param query - GraphQL query to execute.
  * @param token - Business Platform token.
  * @param variables - GraphQL variables to pass to the query.
+ * @param cacheOptions - Cache options for the request. If not present, the request will not be cached.
  * @returns The response of the query of generic type <TResult>.
  */
-export declare function businessPlatformRequestDoc<TResult, TVariables extends Variables>(query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables): Promise<TResult>;
+export declare function businessPlatformRequestDoc<TResult, TVariables extends Variables>(query: TypedDocumentNode<TResult, TVariables>, token: string, variables?: TVariables, cacheOptions?: CacheOptions): Promise<TResult>;
 /**
  * Executes a GraphQL query against the Business Platform Organizations API.
  *
packages/cli-kit/dist/public/node/api/graphql.d.ts
@@ -1,3 +1,5 @@
+import { ConfSchema } from '../../../private/node/conf-store.js';
+import { LocalStorage } from '../local-storage.js';
 import { rawRequest, RequestDocument, Variables } from 'graphql-request';
 import { TypedDocumentNode } from '@graphql-typed-document-node/core';
 export type Exact<T extends {
@@ -9,6 +11,12 @@ export interface GraphQLVariables {
     [key: string]: any;
 }
 export type GraphQLResponse<T> = Awaited<ReturnType<typeof rawRequest<T>>>;
+export interface CacheOptions {
+    cacheTTL: CacheTTL;
+    cacheExtraKey?: string;
+    cacheStore?: LocalStorage<ConfSchema>;
+}
+export type CacheTTL = number;
 interface GraphQLRequestBaseOptions<TResult> {
     api: string;
     url: string;
@@ -17,6 +25,7 @@ interface GraphQLRequestBaseOptions<TResult> {
         [header: string]: string;
     };
     responseOptions?: GraphQLResponseOptions<TResult>;
+    cacheOptions?: CacheOptions;
 }
 export type GraphQLRequestOptions<T> = GraphQLRequestBaseOptions<T> & {
     query: RequestDocument;

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants