Migrate theme delete/update/publish to GraphQL #4623

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

WHY are these changes introduced?

Following the introduction of theme admin GraphQL endpoints, port the CLI to use GraphQL.

Extending what @catlee started here #4541

WHAT is this pull request doing?

Migrate themeDelete, themeUpdate, and themePublish to use the admin graphql for both regular sessions and theme access sessions. Renamed uses of delete/update/publishTheme to themeDelete/Update/Publish for consistency.

Change themeDelete to return boolean (true on success, false on failure) instead of a theme.

To ship after Chris' code gen PR (shipped).

How to test your changes?

shopify theme delete

shopify theme publish

shopify theme rename

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-09T19:14:38Z

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** 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.

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

catlee · 2024-10-16T09:56:14Z

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

-  const response = await request('POST', `/themes`, session, params)
-  return buildTheme(response.json.theme)
+  return buildTheme({
+    id: parseInt((theme.id as unknown as string).split('/').pop() as string, 10),


as a follow-up, what do you think of swapping over the Theme object here to use gids?

we need to convert back and forth between ints and gids in quite a few places. will we still need the plain int id anywhere after we're done migrating?

I'm kind of shocked we don't already have some composeGid and parseGid functions in the repo. Would be good to add them as a part of this transition I think. There's a parseGid you can rip from Hydrogen if you want: https://github.com/Shopify/hydrogen/blob/main/packages/cli/src/lib/gid.ts

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

karreiro

Thanks a lot for this PR, @lucyxiang! It's heading in an excellent direction :)

I've left just a comments about keeping the API layer compatible with the Theme Access app. Otherwise, we might potentially impact CI/CD workflows and users using that as an authentication method.

Also, I believe we could ignore the admin_schema.graphql file, following the reasoning in #4620 (comment).

Thanks again for this PR!

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

github-actions · 2024-10-16T19:50:10Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	72.43% (-0.11% 🔻)	8366/11550
🟡	Branches	68.8% (-0.19% 🔻)	4078/5927
🟡	Functions	71.87% (-0.22% 🔻)	2197/3057
🟡	Lines	72.73% (-0.11% 🔻)	7912/10878

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / theme_delete.ts	100%	100%	100%	100%
🟢	`...` / theme_publish.ts	100%	100%	100%	100%
🟢	`...` / theme_update.ts	100%	100%	100%	100%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟡	`...` / api.ts	61.86% (-21.97% 🔻)	53.06% (-30.81% 🔻)	52% (-29.82% 🔻)	62.22% (-22.15% 🔻)
🟡	`...` / utils.ts	66.67% (+66.67% 🔼)	75% (-25% 🔻)	50% (+50% 🔼)	66.67% (+66.67% 🔼)

Test suite run success

1919 tests passing in 872 suites.

Report generated by 🧪jest coverage report action from a65ac51

github-actions · 2024-10-22T18:20:33Z

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.

graygilmore

No 🎩 yet but looking good! Small concern around the types that are getting generated, would love to confirm that.

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

graygilmore · 2024-10-22T22:12:11Z

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

-  const response = await request('POST', `/themes`, session, params)
-  return buildTheme(response.json.theme)
+  return buildTheme({
+    id: parseInt((theme.id as unknown as string).split('/').pop() as string, 10),


I'm kind of shocked we don't already have some composeGid and parseGid functions in the repo. Would be good to add them as a part of this transition I think. There's a parseGid you can rip from Hydrogen if you want: https://github.com/Shopify/hydrogen/blob/main/packages/cli/src/lib/gid.ts

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

EvilGenius13 · 2024-10-23T14:49:44Z

Quick question: for the mutations, should they be in something like api/graphql/admin/mutations/theme_delete.graphql
instead of api/graphql/admin/queries/theme_delete.graphql since they are mutations and we might have queries at a later point?

lucyxiang · 2024-10-25T16:53:13Z

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

+    } else {
+      // An unexpected error if neither theme nor userErrors are returned
+      unexpectedGraphQLError('Failed to update theme')


Not sure when this would happen (no execution error but also not getting back the specified fields). I was tempted to not have this else but if it does happen, it's better we raise an error than not.

👍🏻 ideally adminRequestDoc covers this case for all calls but we don't have to push that in this PR. I'm still convinced that the types in this repo aren't being generated quite right but I don't have time to poke at it yet. We'll just have to be extra defensive for now I think.

lucyxiang · 2024-10-25T17:06:43Z

packages/theme/src/cli/services/delete.test.ts

@@ -1,6 +1,6 @@
-import {deleteThemes, renderDeprecatedArgsWarning} from './delete.js'


Mostly renaming from here on down (all the changes in this directory)

graygilmore

no 🎩 but code looks good!

graygilmore · 2024-10-25T22:40:28Z

packages/cli-kit/src/public/node/themes/utils.ts

+  return `gid://shopify/OnlineStoreTheme/${id}`
+}
+
+export function themeGidToId(gid: string): number {


Nit: prefer parseGid to match other systems.

(nit in the same vein: themeGid => composeThemeGid but feel free to ignore)

graygilmore · 2024-10-25T22:51:10Z

packages/cli-kit/src/public/node/themes/utils.ts

+}
+
+export function themeGidToId(gid: string): number {
+  return parseInt((gid as unknown as string).split('/').pop() as string, 10)


I believe right now all of our resources should contain a numeric ID on the end of the GID but it's not necessarily a guarantee. I think parseInt() can probably also handle query params (e.g. gid://shopify/Example/1?foo=3&bar=baz but it feels wrong.

I know that this probably works in all scenarios but I wonder if it'd be better feedback if we threw an error if we ever received a GID that wasn't valid. Here's a regex that we've used in other projects:

Suggested change

return parseInt((gid as unknown as string).split('/').pop() as string, 10)

const id = `/${gid}`;

const matches = /\/(\w[\w-]*)(?:\?(.*))*$/.exec(id);

if (matches && matches[1] !== undefined) {

return matches[1];

}

throw new Error(`Invalid GID: ${gid}`);

And the one I mentioned before in Hydrogen: https://github.com/Shopify/hydrogen/blob/main/packages/cli/src/lib/gid.ts

What do you think?

Good to go with what hydrogen uses, it seems more foolproof which I like. Thank you for sharing Gray!

I missed this link in your previous comment, my bad 🙈 didn't mean to ignore it

Once we have all the old REST code removed, I think we can remove most of the GID parsing / generation.

These can mostly be treated as opaque strings, with the exception of how the theme list command outputs them, or passing in themes by id on the command line.

graygilmore · 2024-10-25T23:14:36Z

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

+    } else {
+      // An unexpected error if neither theme nor userErrors are returned
+      unexpectedGraphQLError('Failed to update theme')


👍🏻 ideally adminRequestDoc covers this case for all calls but we don't have to push that in this PR. I'm still convinced that the types in this repo aren't being generated quite right but I don't have time to poke at it yet. We'll just have to be extra defensive for now I think.

graygilmore · 2024-10-25T23:37:44Z

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

 }

-export async function publishTheme(id: number, session: AdminSession): Promise<Theme | undefined> {
-  return updateTheme(id, {role: 'main'}, session)
+export async function themePublish(id: number, session: AdminSession): Promise<Theme | undefined> {


🚲 🖌️ (nit) feel free to ignore completely, just sharing how I might have written this now that I see your new approach!

export async function themePublish(id: number, session: AdminSession): Promise<Theme | undefined> { try { const {themePublish} = await adminRequestDoc(ThemePublish, session, {id: themeGid(id)}) if (!themePublish) throw new Error('') const {theme, userErrors} = themePublish if (userErrors.length) { throw new Error(userErrors.map((error) => error.message).join(', ')) } if (!theme) throw new Error('') return buildTheme({ id: themeGidToId(theme.id), name: theme.name, role: theme.role.toLowerCase(), }) } catch (error: unknown) { if (error instanceof Error) { throw new AbortError(error.message) } throw new AbortError('An unknown error occurred') } }

I like the try/catch here because it means we can just throw when we don't like the state and we don't end up in nested if/else statements.

I used a mix of yours and guilherme's suggestion. I don't love throwing Error('') and then populating it later , but agreed with less if/else statements.

karreiro

Thank you for this PR, @lucyxiang! LGTM and works as expected as well 🎩 I've left two minor comments that I'd like to know your thoughts :)

Thanks again for this PR!

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

github-actions · 2024-10-29T14:47:58Z

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

packages/cli-kit/dist/cli/api/graphql/admin/generated/theme_delete.d.ts

import * as Types from './types.js';
import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core';
export type ThemeDeleteMutationVariables = Types.Exact<{
    id: Types.Scalars['ID']['input'];
}>;
export type ThemeDeleteMutation = {
    themeDelete?: {
        deletedThemeId?: string | null;
        userErrors: {
            field?: string[] | null;
            message: string;
        }[];
    } | null;
};
export declare const ThemeDelete: DocumentNode<ThemeDeleteMutation, Types.Exact<{
    id: Types.Scalars['ID']['input'];
}>>;

packages/cli-kit/dist/cli/api/graphql/admin/generated/theme_publish.d.ts

import * as Types from './types.js';
import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core';
export type ThemePublishMutationVariables = Types.Exact<{
    id: Types.Scalars['ID']['input'];
}>;
export type ThemePublishMutation = {
    themePublish?: {
        theme?: {
            id: string;
            name: string;
            role: Types.ThemeRole;
        } | null;
        userErrors: {
            field?: string[] | null;
            message: string;
        }[];
    } | null;
};
export declare const ThemePublish: DocumentNode<ThemePublishMutation, Types.Exact<{
    id: Types.Scalars['ID']['input'];
}>>;

packages/cli-kit/dist/cli/api/graphql/admin/generated/theme_update.d.ts

import * as Types from './types.js';
import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core';
export type ThemeUpdateMutationVariables = Types.Exact<{
    id: Types.Scalars['ID']['input'];
    input: Types.OnlineStoreThemeInput;
}>;
export type ThemeUpdateMutation = {
    themeUpdate?: {
        theme?: {
            id: string;
            name: string;
            role: Types.ThemeRole;
        } | null;
        userErrors: {
            field?: string[] | null;
            message: string;
        }[];
    } | null;
};
export declare const ThemeUpdate: DocumentNode<ThemeUpdateMutation, Types.Exact<{
    id: Types.Scalars['ID']['input'];
    input: Types.OnlineStoreThemeInput;
}>>;

Existing type declarations

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

@@ -9,14 +9,6 @@ export declare function fetchThemeAsset(id: number, key: Key, session: AdminSess
 export declare function deleteThemeAsset(id: number, key: Key, session: AdminSession): Promise<boolean>;
 export declare function bulkUploadThemeAssets(id: number, assets: AssetParams[], session: AdminSession): Promise<Result[]>;
 export declare function fetchChecksums(id: number, session: AdminSession): Promise<Checksum[]>;
-interface UpgradeThemeOptions {
-    fromTheme: number;
-    toTheme: number;
-    script?: string;
-    session: AdminSession;
-}
-export declare function upgradeTheme(upgradeOptions: UpgradeThemeOptions): Promise<Theme | undefined>;
-export declare function updateTheme(id: number, params: ThemeParams, session: AdminSession): Promise<Theme | undefined>;
-export declare function publishTheme(id: number, session: AdminSession): Promise<Theme | undefined>;
-export declare function deleteTheme(id: number, session: AdminSession): Promise<Theme | undefined>;
-export {};
\ No newline at end of file
+export declare function themeUpdate(id: number, params: ThemeParams, session: AdminSession): Promise<Theme | undefined>;
+export declare function themePublish(id: number, session: AdminSession): Promise<Theme | undefined>;
+export declare function themeDelete(id: number, session: AdminSession): Promise<boolean | undefined>;
\ No newline at end of file

packages/cli-kit/dist/public/node/themes/utils.d.ts

@@ -4,4 +4,6 @@ export declare const LIVE_THEME_ROLE = "live";
 export declare const UNPUBLISHED_THEME_ROLE = "unpublished";
 export type Role = typeof DEVELOPMENT_THEME_ROLE | typeof LIVE_THEME_ROLE | typeof UNPUBLISHED_THEME_ROLE;
 export declare function isDevelopmentTheme(theme: Theme): boolean;
-export declare function promptThemeName(message: string): Promise<string>;
\ No newline at end of file
+export declare function promptThemeName(message: string): Promise<string>;
+export declare function composeThemeGid(id: number): string;
+export declare function parseGid(gid: string): number;
\ No newline at end of file

lucyxiang · 2024-10-29T16:54:47Z

/shipit

gordonhirsch

Sorry for the delay.

lucyxiang requested a review from a team as a code owner October 9, 2024 19:14

lucyxiang requested review from jamieguerrero and gordonhirsch and removed request for a team October 9, 2024 19:14

lucyxiang requested review from catlee and karreiro October 10, 2024 17:22

lucyxiang force-pushed the migrate-theme-delete-theme-publish-to-gql branch from aae05b3 to ca94b5a Compare October 15, 2024 20:49

catlee approved these changes Oct 16, 2024

View reviewed changes

karreiro reviewed Oct 16, 2024

View reviewed changes

packages/cli-kit/src/public/node/themes/api.ts Outdated Show resolved Hide resolved

packages/cli-kit/src/public/node/themes/api.ts Outdated Show resolved Hide resolved

lucyxiang force-pushed the migrate-theme-delete-theme-publish-to-gql branch 2 times, most recently from 8a56f60 to 6ba04ee Compare October 16, 2024 19:45

lucyxiang force-pushed the migrate-theme-delete-theme-publish-to-gql branch from 6ba04ee to 0824a14 Compare October 22, 2024 18:12

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

lucyxiang force-pushed the migrate-theme-delete-theme-publish-to-gql branch from 0824a14 to ae33c65 Compare October 22, 2024 18:20

lucyxiang force-pushed the migrate-theme-delete-theme-publish-to-gql branch from ae33c65 to 2fb46d3 Compare October 22, 2024 18:38

graygilmore reviewed Oct 22, 2024

View reviewed changes

lucyxiang force-pushed the migrate-theme-delete-theme-publish-to-gql branch 3 times, most recently from 4ab70cf to 517c2bf Compare October 24, 2024 20:39

lucyxiang requested a review from karreiro October 24, 2024 20:40

lucyxiang force-pushed the migrate-theme-delete-theme-publish-to-gql branch from 517c2bf to e0976e9 Compare October 25, 2024 16:34

lucyxiang commented Oct 25, 2024

View reviewed changes

lucyxiang force-pushed the migrate-theme-delete-theme-publish-to-gql branch from e0976e9 to 555e722 Compare October 25, 2024 17:08

lucyxiang requested review from graygilmore and jamesmengo October 25, 2024 17:08

graygilmore approved these changes Oct 25, 2024

View reviewed changes

karreiro approved these changes Oct 28, 2024

View reviewed changes

packages/cli-kit/src/public/node/themes/api.ts Outdated Show resolved Hide resolved

packages/cli-kit/src/public/node/themes/api.ts Show resolved Hide resolved

lucyxiang force-pushed the migrate-theme-delete-theme-publish-to-gql branch 2 times, most recently from 4dc5b4a to 6eaa4ac Compare October 29, 2024 14:21

Migrate theme delete/push/update to GraphQL

a65ac51

lucyxiang force-pushed the migrate-theme-delete-theme-publish-to-gql branch from 6eaa4ac to a65ac51 Compare October 29, 2024 14:45

lucyxiang enabled auto-merge October 29, 2024 19:15

gordonhirsch approved these changes Oct 30, 2024

View reviewed changes

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

Merged via the queue into main with commit ce52ab9 Oct 30, 2024
27 checks passed

lucyxiang deleted the migrate-theme-delete-theme-publish-to-gql branch October 30, 2024 15:07

shopify-shipit bot temporarily deployed to nightly October 31, 2024 14:39 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

Migrate theme delete/update/publish to GraphQL #4623

Migrate theme delete/update/publish to GraphQL #4623

lucyxiang commented Oct 9, 2024 •

edited

Loading

github-actions bot commented Oct 9, 2024

catlee Oct 16, 2024

graygilmore Oct 22, 2024

karreiro left a comment

github-actions bot commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 22, 2024

graygilmore left a comment

graygilmore Oct 22, 2024

EvilGenius13 commented Oct 23, 2024

lucyxiang Oct 25, 2024

graygilmore Oct 25, 2024

lucyxiang Oct 25, 2024 •

edited

Loading

graygilmore left a comment

graygilmore Oct 25, 2024

graygilmore Oct 25, 2024

lucyxiang Oct 28, 2024

lucyxiang Oct 28, 2024

catlee Oct 29, 2024

graygilmore Oct 25, 2024

graygilmore Oct 25, 2024

lucyxiang Oct 28, 2024 •

edited

Loading

karreiro left a comment •

edited

Loading

github-actions bot commented Oct 29, 2024

lucyxiang commented Oct 29, 2024

gordonhirsch left a comment

		@@ -1,6 +1,6 @@
		import {deleteThemes, renderDeprecatedArgsWarning} from './delete.js'

-  return parseInt((gid as unknown as string).split('/').pop() as string, 10)
+  const id = `/${gid}`;
+  const matches = /\/(\w[\w-]*)(?:\?(.*))*$/.exec(id);
+  if (matches && matches[1] !== undefined) {
+    return matches[1];
+  }
+  throw new Error(`Invalid GID: ${gid}`);

Migrate theme delete/update/publish to GraphQL #4623

Migrate theme delete/update/publish to GraphQL #4623

Conversation

lucyxiang commented Oct 9, 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 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karreiro left a comment

Choose a reason for hiding this comment

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

Coverage report

Test suite run success

github-actions bot commented Oct 22, 2024

graygilmore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvilGenius13 commented Oct 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyxiang Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

graygilmore 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyxiang Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

karreiro left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 29, 2024

Differences in type declarations

New type declarations

Existing type declarations

lucyxiang commented Oct 29, 2024

gordonhirsch left a comment

Choose a reason for hiding this comment

lucyxiang commented Oct 9, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

lucyxiang Oct 25, 2024 •

edited

Loading

lucyxiang Oct 28, 2024 •

edited

Loading

karreiro left a comment •

edited

Loading