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

Use graphQL to delete theme files #4799

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

Conversation

catlee
Copy link
Contributor

@catlee catlee commented Nov 5, 2024

WHY are these changes introduced?

Use graphQL to delete theme files. This gets us off REST, and also allows us to delete multiple files in one request, which should improve the developer experience.

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

github-actions bot commented Nov 5, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.04% (+0.02% 🔼)
8359/11603
🟡 Branches
68.42% (-0.02% 🔻)
4038/5902
🟡 Functions
71.31% (-0.01% 🔻)
2192/3074
🟡 Lines
72.41% (+0.02% 🔼)
7904/10915
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / theme_files_delete.ts
100% 100% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / api.ts
53.85% (+1.91% 🔼)
44.29% (-2.49% 🔻)
48.48% (+0.1% 🔼)
54.48% (+1.59% 🔼)

Test suite run success

1889 tests passing in 865 suites.

Report generated by 🧪jest coverage report action from be5792f

@catlee catlee changed the title WIP - port over theme files delete Use graphQL to delete theme files Nov 6, 2024
@catlee catlee self-assigned this Nov 6, 2024
@catlee catlee marked this pull request as ready for review November 6, 2024 15:55
@catlee catlee requested review from a team as code owners November 6, 2024 15:55
@catlee catlee requested a review from lucyxiang November 6, 2024 15:55
Copy link
Contributor

github-actions bot commented Nov 6, 2024

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

@graygilmore graygilmore left a comment

Choose a reason for hiding this comment

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

no 🎩 yet but code part looks good, one small question about the progress bar

@@ -179,9 +179,13 @@ async function performFileReconciliation(
)
})

const deleteRemoteFiles = remoteFilesToDelete.map((file) => deleteThemeAsset(targetTheme.id, file.key, session))
const deleteRemoteFiles = deleteThemeAssets(
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be so much faster!

if (!success) {
createSyncingCatchError(key, 'delete')(new Error(`Failed to delete ${key}`))
}
progress.current++
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if there's any point to the progress tracking here now? By the time we're doing progress.current++ all of the remote calls have already been completed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I wasn't sure about that either....

In the case where we have multiple batches, progress tracking could be useful....but the batching is being handled by the underlying function right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented batching in the uploader as well, so if we have multiple batches of files to delete we'll get progress.

I also hooked up the delete errors to the uploadResults so that the CLI reports that the push finished with errors.

@lucyxiang
Copy link
Contributor

lucyxiang commented Nov 6, 2024

While 🎩 I found we run into a nil error here (see observe logs). I guess the CLI (or at least in dev) doesn't have an associated user 🤔

Edit: I can't 🎩 further until this is fixed :/

@catlee catlee force-pushed the catlee/theme_files_delete branch 2 times, most recently from eb32a38 to 5ee226f Compare November 7, 2024 16:19
preserve error message.
Fix up progress and presenting CLI errors on delete.
Copy link
Contributor

github-actions bot commented Nov 7, 2024

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_files_delete.d.ts
import * as Types from './types.js';
import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core';
export type ThemeFilesDeleteMutationVariables = Types.Exact<{
    themeId: Types.Scalars['ID']['input'];
    files: Types.Scalars['String']['input'][] | Types.Scalars['String']['input'];
}>;
export type ThemeFilesDeleteMutation = {
    themeFilesDelete?: {
        deletedThemeFiles?: {
            filename: string;
        }[] | null;
        userErrors: {
            filename?: string | null;
            code?: Types.OnlineStoreThemeFilesUserErrorsCode | null;
            message: string;
        }[];
    } | null;
};
export declare const ThemeFilesDelete: DocumentNode<ThemeFilesDeleteMutation, Types.Exact<{
    themeId: Types.Scalars['ID']['input'];
    files: Types.Scalars['String']['input'][] | Types.Scalars['String']['input'];
}>>;

Existing type declarations

packages/cli-kit/dist/public/node/themes/api.d.ts
@@ -6,7 +6,7 @@ export declare function fetchTheme(id: number, session: AdminSession): Promise<T
 export declare function fetchThemes(session: AdminSession): Promise<Theme[]>;
 export declare function createTheme(params: ThemeParams, session: AdminSession): Promise<Theme | undefined>;
 export declare function fetchThemeAssets(id: number, filenames: Key[], session: AdminSession): Promise<ThemeAsset[]>;
-export declare function deleteThemeAsset(id: number, key: Key, session: AdminSession): Promise<boolean>;
+export declare function deleteThemeAssets(id: number, filenames: Key[], session: AdminSession): Promise<Result[]>;
 export declare function bulkUploadThemeAssets(id: number, assets: AssetParams[], session: AdminSession): Promise<Result[]>;
 export declare function fetchChecksums(id: number, session: AdminSession): Promise<Checksum[]>;
 export declare function themeUpdate(id: number, params: ThemeParams, session: AdminSession): Promise<Theme | undefined>;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants