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

Migrate themeCreate to GraphQL #5249

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

lucyxiang
Copy link
Contributor

@lucyxiang lucyxiang commented Jan 21, 2025

WHY are these changes introduced?

Migrate theme create to use GraphQL themeCreate mutation. Use skeleton theme (minimal theme that contains only theme.liquid, password.liquid, and settings_schema.json) created my @jamesmengo

How to test your changes?

shopify theme dev or shopify theme push

  • running with theme directory creates a skeleton theme and pushes those file
  • running without theme directory just creates a skeleton theme
Screenshot 2025-02-06 at 13 32 40

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

@lucyxiang lucyxiang changed the title wip Migrate themeCreate to GraphQL Jan 21, 2025
Copy link
Contributor

github-actions bot commented Jan 21, 2025

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
75.56% (-0.09% 🔻)
9027/11947
🟡 Branches
70.93% (+0.05% 🔼)
4411/6219
🟡 Functions
75.22% (-0.2% 🔻)
2362/3140
🟡 Lines
76.07% (-0.08% 🔻)
8523/11204
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / theme_create.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟢
... / app-event-watcher.ts
95.18% (-1.2% 🔻)
86.49% (-2.7% 🔻)
95.45% 100%
🔴
... / app-management-client.ts
34.55% (-0.9% 🔻)
27.08% (-0.58% 🔻)
34.69% (-0.36% 🔻)
33.6% (-0.82% 🔻)
🟡
... / rest-api-throttler.ts
62.37% (-26.88% 🔻)
50% (-30% 🔻)
46.67% (-36.67% 🔻)
62.37% (-26.88% 🔻)

Test suite run success

2045 tests passing in 913 suites.

Report generated by 🧪jest coverage report action from 4a3895c

@lucyxiang lucyxiang force-pushed the migrate-theme-create-to-gql branch 3 times, most recently from f0c0b74 to 2a64370 Compare February 5, 2025 21:24
@lucyxiang lucyxiang marked this pull request as ready for review February 6, 2025 18:12
@lucyxiang lucyxiang requested review from a team as code owners February 6, 2025 18:12

This comment has been minimized.

@lucyxiang lucyxiang force-pushed the migrate-theme-create-to-gql branch from 2a64370 to dcb5814 Compare February 6, 2025 18:39
@@ -406,135 +419,10 @@ export async function passwordProtected(session: AdminSession): Promise<boolean>
return passwordProtection.enabled
}

async function request<T>(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🔥 🔥 🔥

@lucyxiang lucyxiang force-pushed the migrate-theme-create-to-gql branch from dcb5814 to 1fa637d Compare February 6, 2025 18:46
source: themeSource,
role: (params.role ?? DEVELOPMENT_THEME_ROLE).toUpperCase() as ThemeRole,
},
'2025-04',
Copy link
Contributor Author

@lucyxiang lucyxiang Feb 6, 2025

Choose a reason for hiding this comment

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

To remove post RC 2025-04. role argument was recently added and is only in 2025-04

Copy link
Contributor

Choose a reason for hiding this comment

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

How will we remember to do this?

Copy link
Contributor Author

@lucyxiang lucyxiang Feb 11, 2025

Choose a reason for hiding this comment

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

Currently I just have it as a cal reminder for april 1st 😅 Unless there's a remind me / todo func in the CLI repo?

Copy link
Contributor

Choose a reason for hiding this comment

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

You may be able to schedule an issue via a workflow but that feels like overkill 🤷🏼

@lucyxiang lucyxiang force-pushed the migrate-theme-create-to-gql branch from 1fa637d to 3a3dc8e Compare February 7, 2025 21:06
import {GetThemeFileChecksums} from '../../../cli/api/graphql/admin/generated/get_theme_file_checksums.js'
import {ThemeFilesUpsert} from '../../../cli/api/graphql/admin/generated/theme_files_upsert.js'
import {ThemeFilesDelete} from '../../../cli/api/graphql/admin/generated/theme_files_delete.js'
import {OnlineStoreThemeFileBodyInputType} from '../../../cli/api/graphql/admin/generated/types.js'
import {GetThemes} from '../../../cli/api/graphql/admin/generated/get_themes.js'
import {GetTheme} from '../../../cli/api/graphql/admin/generated/get_theme.js'
import {test, vi, expect, describe} from 'vitest'
import {adminRequestDoc, restRequest, supportedApiVersions} from '@shopify/cli-kit/node/api/admin'
Copy link
Contributor

Choose a reason for hiding this comment

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

oooh. 🔥

source: themeSource,
role: (params.role ?? DEVELOPMENT_THEME_ROLE).toUpperCase() as ThemeRole,
},
'2025-04',
Copy link
Contributor

Choose a reason for hiding this comment

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

How will we remember to do this?

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

packages/cli-kit/dist/cli/api/graphql/admin/generated/theme_create.d.ts
import * as Types from './types.js';
import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core';
export type ThemeCreateMutationVariables = Types.Exact<{
    name: Types.Scalars['String']['input'];
    source: Types.Scalars['URL']['input'];
    role: Types.ThemeRole;
}>;
export type ThemeCreateMutation = {
    themeCreate?: {
        theme?: {
            id: string;
            name: string;
            role: Types.ThemeRole;
        } | null;
        userErrors: {
            field?: string[] | null;
            message: string;
        }[];
    } | null;
};
export declare const ThemeCreate: DocumentNode<ThemeCreateMutation, ThemeCreateMutationVariables>;

Existing type declarations

packages/cli-kit/dist/public/node/themes/api.d.ts
@@ -5,7 +5,7 @@ export type ThemeParams = Partial<Pick<Theme, 'name' | 'role' | 'processing' | '
 export type AssetParams = Pick<ThemeAsset, 'key'> & Partial<Pick<ThemeAsset, 'value' | 'attachment'>>;
 export declare function fetchTheme(id: number, session: AdminSession): Promise<Theme | undefined>;
 export declare function fetchThemes(session: AdminSession): Promise<Theme[]>;
-export declare function createTheme(params: ThemeParams, session: AdminSession): Promise<Theme | undefined>;
+export declare function themeCreate(params: ThemeParams, session: AdminSession): Promise<Theme | undefined>;
 export declare function fetchThemeAssets(id: number, filenames: Key[], session: AdminSession): Promise<ThemeAsset[]>;
 export declare function deleteThemeAssets(id: number, filenames: Key[], session: AdminSession): Promise<Result[]>;
 export declare function bulkUploadThemeAssets(id: number, assets: AssetParams[], session: AdminSession): Promise<Result[]>;

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.

4 participants