Add GQL support for theme access tokens #4717

lucyxiang · 2024-10-22T18:10:59Z

WHY are these changes introduced?

Along with https://github.com/Shopify/themekit-access/pull/3703, add support for theme access tokens for GraphQL in the CLI. Will unblocking CLI GraphQL migration effort.

WHAT is this pull request doing?

Add theme access logic for GraphQL requests (proxy to theme access app with appropriate headers).

In follow up, would like to clean up theme access logic by creating a file for it, rather than implementing it in REST and GraphQL

How to test your changes?

See https://github.com/Shopify/themekit-access/pull/3703 for tophatting instructions

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-22T18:12:32Z

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.

lucyxiang · 2024-10-22T18:24:33Z

packages/cli-kit/src/private/node/api.ts

@@ -70,7 +70,7 @@ async function makeVerboseRequest<T extends {headers: Headers; status: number}>(
      }
      const sanitizedHeaders = sanitizedHeadersOutput(responseHeaders)

-      if (err.response.errors?.some((error) => error.extensions.code === '429') || err.response.status === 429) {
+      if (err.response.status === 429 || errorsInclude429(err)) {


Unrelated to the functionality of this PR, I noticed sometimes err.response.errors is a string so this was raising (can't call .some on a string). Now I check that errors.response.errors is not a string before calling .some to look through its contents

Can you help me understand how this could be the case? Is graphql-request (the package we get this type from) outdated?

~~I know the string error message is coming from core. Maybe something about proxying this request through theme access app, we aren't respecting the expected gql return type?~~ Hmm I will investigate

GraphQL returns a 401 with this Invalid API key or access token (unrecognized login or wrong password) error message when auth fails (code). So we do expect that this error can be a string.

I've updated the GraphQLResponse type so that errors can be a string or GraphQLError[].

packages/cli-kit/src/private/node/api/graphql.ts

github-actions · 2024-10-22T19:30:30Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	72.6% (-0.02% 🔻)	8437/11622
🟡	Branches	69.15% (+0.02% 🔼)	4149/6000
🟡	Functions	72.03% (-0.03% 🔻)	2204/3060
🟡	Lines	72.87% (-0.02% 🔻)	7979/10950

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / loader.ts	93.05% (-0.04% 🔻)	83.52% (+0.19% 🔼)	97.03%	93.97% (-0.03% 🔻)
🟡	`...` / update-extension.ts	70% (-1.79% 🔻)	58.33%	60%	74.29% (-2.18% 🔻)
🟡	`...` / api.ts	67.09% (-0.48% 🔻)	44% (+0.52% 🔼)	77.78% (+2.78% 🔼)	67.11% (-0.5% 🔻)
🟢	`...` / rest.ts	86.67% (-0.83% 🔻)	71.43% (-8.57% 🔻)	80%	86.67% (-0.83% 🔻)
🟢	`...` / is-global.ts	80.65% (-0.6% 🔻)	69.23%	80%	88% (-0.46% 🔻)
🟢	`...` / node-package-manager.ts	86.19% (-0.55% 🔻)	84.44%	86.11% (-2.78% 🔻)	86.03% (-0.56% 🔻)

Test suite run success

1931 tests passing in 874 suites.

Report generated by 🧪jest coverage report action from f747f67

lucyxiang · 2024-10-23T18:36:47Z

packages/cli-kit/src/private/node/api/headers.ts

@@ -56,7 +56,7 @@ export function buildHeaders(token?: string): {[key: string]: string} {
    ...(firstPartyDev() && {'X-Shopify-Cli-Employee': '1'}),
  }
  if (token) {
-    const authString = token.match(/^shp(at|ua|ca)/) ? token : `Bearer ${token}`
+    const authString = token.match(/^shp(at|ua|ca|tka)/) ? token : `Bearer ${token}`


shptka_* represent theme access tokens, and we want those as is, not Bearer ${token}

lucyxiang · 2024-10-23T19:06:14Z

packages/cli-kit/src/private/node/api.ts

@@ -70,7 +70,7 @@ async function makeVerboseRequest<T extends {headers: Headers; status: number}>(
      }
      const sanitizedHeaders = sanitizedHeadersOutput(responseHeaders)

-      if (err.response.errors?.some((error) => error.extensions.code === '429') || err.response.status === 429) {
+      if (errorsIncludeStatus429(err)) {


lucyxiang · 2024-10-23T19:07:27Z

packages/cli-kit/src/private/node/api.ts

+  if (typeof error.response.errors === 'string') {
+    return false
+  }
+  return error.response.errors?.some((error) => error.extensions?.code === '429') ?? false
+}


My TS isn't the best, ideally I want to do something like this but it didn't seem possible

if (typeof error.response.errors === 'GraphQL[]'){ return error.response.errors?.some((error) => error.extensions?.code === '429') ?? false }

I don't think you can interpret array types at run time, so this is probably fine. This error class is a bit of a mess anyway 🙈

I think the type checking here will fix this issue. Thanks!

I don't have access to this bugsnag, but I assume this is the error.response.errors?.some is not a function or something error I encountered as well?

alexanderMontague

I think this mostly looks great! I'll defer to the CLI experts but my only feedback would be to abstract the graphql config. Nice work!

alexanderMontague · 2024-10-23T20:25:17Z

packages/cli-kit/src/private/node/api.ts

+    return true
+  }
+
+  // More so checking if type of error.response.errors is not GraphQLError[]


This is not actually typed within this error class as possibly being a string ((property) GraphQLResponse<unknown>.errors?: GraphQLError[] | undefined) and doing so might open up a whole new can worms with invalid typing throughout the repo (although you could try), so leaving a comment with the context you provided in this comment would be valuable!

Gotcha, I had updated the GraphQLResponse type to be string | GraphQLError[] locally but just realized it's a node module and not part of the CLI. I'll leave it as a comment for now

alexanderMontague · 2024-10-23T20:32:06Z

packages/cli-kit/src/private/node/api.ts

+  if (typeof error.response.errors === 'string') {
+    return false
+  }
+  return error.response.errors?.some((error) => error.extensions?.code === '429') ?? false
+}


I don't think you can interpret array types at run time, so this is probably fine. This error class is a bit of a mess anyway 🙈

packages/cli-kit/src/private/node/api/graphql.ts

alexanderMontague · 2024-10-23T21:04:01Z

packages/cli-kit/src/public/node/api/admin.ts

  const realVersion = version ?? 'unstable'
-  return `https://${store}/admin/api/${realVersion}/graphql.json`
+  const themeKitAccessDomain = process.env[environmentVariables.themeKitAccessDomain] ?? defaultThemeKitAccessDomain


I want to say this should maybe be defined in here?

alexanderMontague · 2024-10-23T21:07:43Z

packages/cli-kit/src/public/node/api/admin.ts

+  const themeKitAccessDomain = process.env[environmentVariables.themeKitAccessDomain] ?? defaultThemeKitAccessDomain
+
+  const url =
+    session && isThemeAccessSession(session)


It seems we're "sprinkling" isThemeAccessSession around in various places. It might be best to lift this up to admin.ts at least so we can craft the URL from either adminUrl or something like themeKitUrl(). @shauns made a nice slack post on this thinking here

Yes in the issue description I mentioned In follow up, would like to clean up theme access logic by creating a file for it, rather than implementing it in REST and GraphQL. I was thinking of doing it as a fast follow since this PR is blocking other CLI GraphQL PRs but I can also try to do it here

Thats fair, so long as we can track it with an issue! Thanks

lucyxiang · 2024-10-24T18:41:18Z

packages/cli-kit/src/public/node/api/admin.ts

  }
  const result = graphqlRequestDoc<TResult, TVariables>({...opts, query, variables, responseOptions})
  return result
 }

+function headers(session: AdminSession): {[header: string]: string} {


Suggested change

function headers(session: AdminSession): {[header: string]: string} {

function themeAccessHeaders(session: AdminSession): {[header: string]: string} {

graygilmore

no 🎩 but everything looks good to me!

github-actions · 2024-10-24T18:55:56Z

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/private/node/constants.d.ts

@@ -58,4 +58,5 @@ export declare const reportingRateLimit: {
     timeout: {
         days: number;
     };
-};
\ No newline at end of file
+};
+export declare const themeKitAccessDomain: string;
\ No newline at end of file

packages/cli-kit/dist/private/node/api/graphql.d.ts

@@ -1,5 +1,5 @@
-import { RequestDocument, Variables } from 'graphql-request';
-export declare function debugLogRequestInfo(api: string, query: RequestDocument, variables?: Variables, headers?: {
+import { Variables } from 'graphql-request';
+export declare function debugLogRequestInfo(api: string, query: string, url: string, variables?: Variables, headers?: {
     [key: string]: string;
 }): void;
 export declare function errorHandler(api: string): (error: unknown, requestId?: string) => unknown;
\ No newline at end of file

packages/cli-kit/dist/private/node/api/rest.d.ts

@@ -1,9 +1,8 @@
-/// <reference types="node" resolution-mode="require"/>
 import { AdminSession } from '@shopify/cli-kit/node/session';
 export declare function restRequestBody<T>(requestBody?: T): string | undefined;
 export declare function restRequestUrl(session: AdminSession, apiVersion: string, path: string, searchParams?: {
     [name: string]: string;
-}, env?: NodeJS.ProcessEnv): string;
+}): string;
 export declare function restRequestHeaders(session: AdminSession): {
     [key: string]: string;
 };

packages/cli-kit/dist/public/node/api/admin.d.ts

@@ -34,9 +34,10 @@ export declare function supportedApiVersions(session: AdminSession): Promise<str
  *
  * @param store - Store FQDN.
  * @param version - API version.
+ * @param session - User session.
  * @returns - Admin API URL.
  */
-export declare function adminUrl(store: string, version: string | undefined): string;
+export declare function adminUrl(store: string, version: string | undefined, session?: AdminSession): string;
 /**
  * Executes a REST request against the Admin API.
  *

lucyxiang requested a review from a team as a code owner October 22, 2024 18:10

lucyxiang force-pushed the add-gql-support-for-themeaccess branch from 95b1089 to 6144fc8 Compare October 22, 2024 18:12

lucyxiang mentioned this pull request Oct 22, 2024

Migrate theme delete/update/publish to GraphQL #4623

Merged

5 tasks

lucyxiang commented Oct 22, 2024

View reviewed changes

packages/cli-kit/src/private/node/api/graphql.ts Outdated Show resolved Hide resolved

lucyxiang force-pushed the add-gql-support-for-themeaccess branch from 6144fc8 to 285746a Compare October 22, 2024 18:32

lucyxiang requested review from karreiro and jamesmengo October 22, 2024 18:42

lucyxiang force-pushed the add-gql-support-for-themeaccess branch from 285746a to 0808e41 Compare October 22, 2024 19:26

lucyxiang force-pushed the add-gql-support-for-themeaccess branch 3 times, most recently from d39cf9f to 51dd61b Compare October 23, 2024 18:05

lucyxiang commented Oct 23, 2024

View reviewed changes

lucyxiang force-pushed the add-gql-support-for-themeaccess branch 2 times, most recently from 074e2c5 to fddc4a5 Compare October 23, 2024 18:44

Add GQL support for theme access tokens

3bc66ee

lucyxiang force-pushed the add-gql-support-for-themeaccess branch from fddc4a5 to 3bc66ee Compare October 23, 2024 18:57

lucyxiang commented Oct 23, 2024

View reviewed changes

alexanderMontague reviewed Oct 23, 2024

View reviewed changes

lucyxiang mentioned this pull request Oct 24, 2024

Refactor theme access kit url support to be consistent across rest and GraphQL #4739

Open

alexanderMontague approved these changes Oct 24, 2024

View reviewed changes

lucyxiang force-pushed the add-gql-support-for-themeaccess branch from 0d36687 to 19b1aa5 Compare October 24, 2024 15:31

lucyxiang commented Oct 24, 2024

View reviewed changes

graygilmore approved these changes Oct 24, 2024

View reviewed changes

jamesmengo approved these changes Oct 24, 2024

View reviewed changes

Address comments

f747f67

lucyxiang force-pushed the add-gql-support-for-themeaccess branch from 19b1aa5 to f747f67 Compare October 24, 2024 18:53

lucyxiang added this pull request to the merge queue Oct 24, 2024

Merged via the queue into main with commit 2720263 Oct 24, 2024
27 checks passed

lucyxiang deleted the add-gql-support-for-themeaccess branch October 24, 2024 19:10

shopify-shipit bot temporarily deployed to nightly October 25, 2024 13:13 Inactive

shopify-shipit bot temporarily deployed to production November 12, 2024 14:35 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GQL support for theme access tokens #4717

Add GQL support for theme access tokens #4717

lucyxiang commented Oct 22, 2024

github-actions bot commented Oct 22, 2024

lucyxiang Oct 22, 2024 •

edited

Loading

jamesmengo Oct 22, 2024

lucyxiang Oct 23, 2024 •

edited

Loading

lucyxiang Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024 •

edited

Loading

lucyxiang Oct 23, 2024

lucyxiang Oct 23, 2024

lucyxiang Oct 23, 2024 •

edited

Loading

alexanderMontague Oct 23, 2024

gonzaloriestra Oct 24, 2024

lucyxiang Oct 24, 2024

alexanderMontague left a comment

alexanderMontague Oct 23, 2024

lucyxiang Oct 24, 2024

alexanderMontague Oct 23, 2024

alexanderMontague Oct 23, 2024

alexanderMontague Oct 23, 2024

lucyxiang Oct 24, 2024 •

edited

Loading

alexanderMontague Oct 24, 2024

lucyxiang Oct 24, 2024

lucyxiang Oct 24, 2024 •

edited

Loading

graygilmore left a comment

github-actions bot commented Oct 24, 2024

	function headers(session: AdminSession): {[header: string]: string} {
	function themeAccessHeaders(session: AdminSession): {[header: string]: string} {

Add GQL support for theme access tokens #4717

Add GQL support for theme access tokens #4717

Conversation

lucyxiang commented Oct 22, 2024

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Measuring impact

Checklist

github-actions bot commented Oct 22, 2024

lucyxiang Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyxiang Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

lucyxiang Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

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

Coverage report

Test suite run success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyxiang Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderMontague left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyxiang Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyxiang Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

graygilmore left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 24, 2024

Differences in type declarations

New type declarations

Existing type declarations

lucyxiang Oct 22, 2024 •

edited

Loading

lucyxiang Oct 23, 2024 •

edited

Loading

lucyxiang Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024 •

edited

Loading

lucyxiang Oct 23, 2024 •

edited

Loading

lucyxiang Oct 24, 2024 •

edited

Loading

lucyxiang Oct 24, 2024 •

edited

Loading