[Theme] Speed up theme synchronization #4362

frandiox · 2024-08-21T19:29:35Z

I think this speeds up the dev command startup by a big margin:

Make password check async
Make checksum async while
Upload file batches in parallel (relying on API throttler) instead of 1 by 1
Delete files in parallel (relying on API throttler) instead of 1 by 1
File deletion is made async so that the server can start before finishing

I think this fixes a bug in file batching because we were not using the base64 size of certain files.
Prevents re-reading files from disk because we can reuse the content that was read beforehand to calculate checksums.

Still need to defer certain upload files. That might come in a different PR.
To review, it might be easier commit by commit.

github-actions · 2024-08-21T19:29:48Z

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.

github-actions · 2024-08-21T19:36:11Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	72.81% (+0.1% 🔼)	8248/11328
🟡	Branches	69.46% (+0.03% 🔼)	4021/5789
🟡	Functions	71.5% (+0.03% 🔼)	2152/3010
🟡	Lines	73.16% (+0.1% 🔼)	7803/10666

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / ConcurrentOutput.tsx	98.39% (-1.61% 🔻)	90.91% (-4.55% 🔻)	100%	98.33% (-1.67% 🔻)

Test suite run success

1860 tests passing in 845 suites.

Report generated by 🧪jest coverage report action from 3ef4cc3

jamesmengo · 2024-08-22T01:03:08Z

packages/theme/src/cli/services/dev.ts

+  const storefrontPassword = await storefrontPasswordPromise
+  session.storefrontPassword = storefrontPassword
+
+  await renderProgress()


👍🏻 great idea!

How do you feel about a rename to clarify that this is unrelated to the renderLinks call below?

Renamed to renderThemeSyncProgress 👍

github-actions · 2024-08-22T11:35: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

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/common/version.d.ts

@@ -1 +1 @@
-export declare const CLI_KIT_VERSION = "3.66.0";
\ No newline at end of file
+export declare const CLI_KIT_VERSION = "3.65.0";
\ No newline at end of file

packages/cli-kit/dist/public/node/http.d.ts

@@ -1,13 +1,12 @@
 import FormData from 'form-data';
-import nodeFetch, { RequestInfo, RequestInit } from 'node-fetch';
-export { FetchError, Request } from 'node-fetch';
+import { RequestInfo, RequestInit, Response } from 'node-fetch';
+export { FetchError, Request, Response } from 'node-fetch';
 /**
  * Create a new FormData object.
  *
  * @returns A FormData object.
  */
 export declare function formData(): FormData;
-export type Response = Awaited<ReturnType<typeof nodeFetch>>;
 /**
  * An interface that abstracts way node-fetch. When Node has built-in
  * support for "fetch" in the standard library, we can drop the node-fetch

packages/cli-kit/dist/public/node/system.d.ts

@@ -1,5 +1,8 @@
 /// <reference types="node" resolution-mode="require"/>
+/// <reference types="node" resolution-mode="require"/>
+/// <reference types="node" resolution-mode="require"/>
 import { AbortSignal } from './abort.js';
+import { ReadStream } from 'tty';
 import type { Writable, Readable } from 'stream';
 export interface ExecOptions {
     cwd?: string;
@@ -45,8 +48,11 @@ export declare function exec(command: string, args: string[], options?: ExecOpti
  */
 export declare function sleep(seconds: number): Promise<void>;
 /**
- * Check if the standard input and output streams support prompting.
+ * In case an standard input stream is passed check if it supports raw mode. Otherwise default standard input stream
+ * will be used.
  *
- * @returns True if the standard input and output streams support prompting.
+ * @param stdin - The standard input stream to check.
+ * @param env - Environmemnt variables.
+ * @returns True in the selected input stream support raw mode.
  */
-export declare function terminalSupportsPrompting(): boolean;
\ No newline at end of file
+export declare function terminalSupportsRawMode(stdin?: ReadStream, env?: NodeJS.ProcessEnv): boolean;
\ No newline at end of file

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

@@ -24,6 +24,7 @@ export interface RemoteBulkUploadResponse {
 }
 export declare function buildTheme(themeJson?: RemoteThemeResponse): Theme | undefined;
 export declare function buildChecksum(asset?: RemoteAssetResponse): Checksum | undefined;
-export declare function buildThemeAsset(asset?: RemoteAssetResponse): ThemeAsset | undefined;
+export declare function buildThemeAsset(asset: undefined): undefined;
+export declare function buildThemeAsset(asset: RemoteAssetResponse): ThemeAsset;
 export declare function buildBulkUploadResults(bulkUploadResponse: RemoteBulkUploadResponse[], assets: AssetParams[]): Result[];
 export {};
\ No newline at end of file

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

@@ -1,8 +1,27 @@
 /// <reference types="node" resolution-mode="require"/>
+/// <reference types="node" resolution-mode="require"/>
+import type { Stats } from 'fs';
 /**
  * {@link Key} represents the unique identifier of a file in a theme.
  */
 export type Key = string;
+export type ThemeFSEventName = 'add' | 'change' | 'unlink';
+interface ThemeFSEventCommonPayload {
+    fileKey: Key;
+    onSync: (fn: () => void) => void;
+}
+type ThemeFSEvent = {
+    type: 'unlink';
+    payload: ThemeFSEventCommonPayload;
+} | {
+    type: 'add' | 'change';
+    payload: ThemeFSEventCommonPayload & {
+        onContent: (fn: (content: string) => void) => void;
+    };
+};
+export type ThemeFSEventPayload<T extends ThemeFSEventName = 'add'> = (ThemeFSEvent & {
+    type: T;
+})['payload'];
 /**
  * Represents a theme on the file system.
  */
@@ -15,12 +34,16 @@ export interface ThemeFileSystem {
      * Local theme files.
      */
     files: Map<Key, ThemeAsset>;
+    /**
+     * Promise that resolves when all the initial files are found.
+     */
+    ready: () => Promise<void>;
     /**
      * Removes a file from the local disk and updates the themeFileSystem
      *
-     * @param assetKey - The key of the file to remove
+     * @param fileKey - The key of the file to remove
      */
-    delete: (assetKey: string) => Promise<void>;
+    delete: (fileKey: Key) => Promise<void>;
     /**
      * Writes a file to the local disk and updates the themeFileSystem
      *
@@ -32,9 +55,22 @@ export interface ThemeFileSystem {
      * Returns a ThemeAsset representing the file that was read
      * Returns undefined if the file does not exist
      *
-     * @param assetKey - The key of the file to read
+     * @param fileKey - The key of the file to read
      */
-    read: (assetKey: string) => Promise<string | Buffer | undefined>;
+    read: (fileKey: Key) => Promise<string | Buffer | undefined>;
+    /**
+     * Gets the stats of a file from the local disk and updates the themeFileSystem
+     * Returns undefined if the file does not exist
+     *
+     * @param fileKey - The key of the file to read
+     */
+    stat: (fileKey: Key) => Promise<Pick<Stats, 'mtime' | 'size'> | undefined>;
+    /**
+     * Add callbacks to run after certain events are fired.
+     */
+    addEventListener: {
+        <T extends ThemeFSEventName>(eventName: T, cb: (params: ThemeFSEventPayload<T>) => void): void;
+    };
 }
 /**
  * Represents a theme.
@@ -117,4 +153,5 @@ export interface Result {
 export declare enum Operation {
     Delete = "DELETE",
     Upload = "UPLOAD"
-}
\ No newline at end of file
+}
+export {};
\ No newline at end of file

jamesmengo · 2024-08-22T17:09:41Z

packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts

 vi.mock('./storefront-renderer.js')

+// Vitest is resetting this mock between tests due to a global config `mockReset: true`.


I couldn't get this working either - Please update me if you figure out another way around this!

jamesmengo

🎩 LGTM + code and works as expected 👍🏻

frandiox · 2024-08-22T19:50:20Z

@jamesmengo I've made further changes that speed up the startup a lot. Please give it another check 🙏

frandiox · 2024-08-22T19:56:02Z

packages/theme/src/cli/utilities/theme-uploader.ts

-}
-
-async function createBatches(files: Checksum[], path: string): Promise<FileBatch[]> {
-  const fileSizes = await Promise.all(files.map((file) => fileSize(`${path}/${file.key}`)))


This was not considering that we upload certain assets coded in base64, so I think batching could be wrong sometimes. It now takes the size of the base64 string for those assets. -- Plus it's now synchronous and doesn't need to read file size again.

Thank you for cleaning this up!
This has been an area of the codebase I was not proud of and had been hoping to revisit for a while so I'm happy to see these improvements :)

No problem at all! This was a tricky thing to reason about, I was lucky to find it 😅

frandiox · 2024-08-22T20:03:32Z

packages/theme/src/cli/utilities/theme-uploader.ts

+        const result = await Promise.race([
+          promise,
+          new Promise((resolve) => setTimeout(() => resolve('timeout'), timeout)),
+        ])
+
+        if (result === 'timeout') {
+          addNextCheck()
+        }


This adds a new task when the timeout ends until promise is resolved. This way we update the title with a progress %

jamesmengo · 2024-08-22T20:18:45Z

packages/theme/src/cli/services/push.ts

@@ -34,13 +34,21 @@ export async function push(theme: Theme, session: AdminSession, options: PushOpt
  const themeChecksums = await fetchChecksums(theme.id, session)
  const themeFileSystem = await mountThemeFileSystem(options.path)

-  const results = await uploadTheme(theme, session, themeChecksums, themeFileSystem, options)
+  const {uploadResults, renderThemeSyncProgress} = await uploadTheme(


This isn't related to the changes in this PR, but this method is hanging / not terminating when I 🎩!

These changes will resolve that issue for us - specifically this commit

Let's make sure that we're able to merge the changes / commit I linked above before the next to avoid breaking push

I don't see the changes to fix this in your links 🤔
In any case, this problem should be fixed here: e3f82ff

Basically, we only defer the delete job if specified in the function, so that the push command can await for everything. Can you check it? 🙏

Sorry - didn't see this. Push and pull are both working properly 👌🏻 !

jamesmengo

Left a comment here to make sure we're on the same page, but no changes needed on this particular changeset as the PR will resolve with this

frandiox · 2024-08-23T10:05:27Z

I've force-pushed to remove the last 2 commits and move them to this follow-up PR: #4370

I think this can be merged as is and will continue in that other PR after also mergin James' changes into main branch.

karreiro

Thank you for this PR, @frandiox! This is awesome 🔥 It's 5x faster in my tests.

I've left one concern that might make the upload process a bit slower, but it would still be faster than the version we have on main. Still, I believe it's worth handling that concern to ensure we keep the uploader stable. Please let me know your thoughts about that :)

Thanks again for the amazing work!

packages/theme/src/cli/utilities/theme-uploader.ts

karreiro

Thank you, @frandiox! 🚀

frandiox added 2 commits August 21, 2024 19:29

Delay fetching remote checksums

e341344

Delay fetching password

011190e

frandiox requested review from karreiro and jamesmengo August 21, 2024 19:29

Cleanup

5e0fc45

jamesmengo reviewed Aug 22, 2024

View reviewed changes

Rename renderThemeSyncProgress

1505172

Base automatically changed from fd-theme-dev to main August 22, 2024 13:47

frandiox added 2 commits August 22, 2024 16:05

Further split files to sync

b603768

Merge branch 'main' into fd-theme-dev-async

cb9a279

jamesmengo reviewed Aug 22, 2024

View reviewed changes

jamesmengo approved these changes Aug 22, 2024

View reviewed changes

frandiox added 5 commits August 22, 2024 20:09

Start delete requests earlier and rely on API throttling

d7bbcc6

Reuse size info to avoid async code

7c02472

Remove unneeded function

2f45deb

Parallelize uploading file batches

2c0c71c

Defer deleting files

8ed8645

frandiox changed the title ~~[Theme] Defer checksums and password checks~~ [Theme] Speed up them synchronization Aug 22, 2024

frandiox requested a review from jamesmengo August 22, 2024 19:49

frandiox changed the title ~~[Theme] Speed up them synchronization~~ [Theme] Speed up theme synchronization Aug 22, 2024

frandiox commented Aug 22, 2024

View reviewed changes

jamesmengo reviewed Aug 22, 2024

View reviewed changes

jamesmengo approved these changes Aug 22, 2024

View reviewed changes

Only defer delete files in server start

e3f82ff

frandiox force-pushed the fd-theme-dev-async branch from 3c363ff to e3f82ff Compare August 23, 2024 10:02

frandiox mentioned this pull request Aug 23, 2024

[Theme] Defer files to upload before server start #4370

Merged

karreiro reviewed Aug 23, 2024

View reviewed changes

packages/theme/src/cli/utilities/theme-uploader.ts Outdated Show resolved Hide resolved

frandiox added 4 commits August 26, 2024 14:24

Upload dependent files in order

4437371

Do not await for deleteJob to start

4ecd3ad

Cleanup

16ec567

Fix 404 page

b8ad836

karreiro approved these changes Aug 26, 2024

View reviewed changes

frandiox added 4 commits August 26, 2024 19:51

More deferring; show progress for theme editor sync

6288d6d

Fix test types

4473787

Restore reportFailedUploads

733037c

Merge branch 'main' into fd-theme-dev-async

3ef4cc3

frandiox added this pull request to the merge queue Aug 27, 2024

Merged via the queue into main with commit 88233bc Aug 27, 2024
36 checks passed

frandiox deleted the fd-theme-dev-async branch August 27, 2024 13:01

frandiox added the #gsd:40767 Fortify local development experience for Liquid themes label Aug 27, 2024

shopify-shipit bot temporarily deployed to nightly August 27, 2024 17:06 Inactive

shopify-shipit bot temporarily deployed to experimental September 3, 2024 08:24 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Theme] Speed up theme synchronization #4362

[Theme] Speed up theme synchronization #4362

frandiox commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024

github-actions bot commented Aug 21, 2024 •

edited

Loading

jamesmengo Aug 22, 2024

frandiox Aug 22, 2024

github-actions bot commented Aug 22, 2024

jamesmengo Aug 22, 2024

jamesmengo left a comment

frandiox commented Aug 22, 2024

frandiox Aug 22, 2024 •

edited

Loading

jamesmengo Aug 22, 2024

frandiox Aug 23, 2024

frandiox Aug 22, 2024

jamesmengo Aug 22, 2024 •

edited

Loading

frandiox Aug 23, 2024

jamesmengo Aug 27, 2024

jamesmengo left a comment •

edited

Loading

frandiox commented Aug 23, 2024

karreiro left a comment

karreiro left a comment

		vi.mock('./storefront-renderer.js')

		// Vitest is resetting this mock between tests due to a global config `mockReset: true`.

[Theme] Speed up theme synchronization #4362

[Theme] Speed up theme synchronization #4362

Conversation

frandiox commented Aug 21, 2024 • edited Loading

github-actions bot commented Aug 21, 2024

github-actions bot commented Aug 21, 2024 • edited Loading

Coverage report

Test suite run success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 22, 2024

Differences in type declarations

New type declarations

Existing type declarations

Choose a reason for hiding this comment

jamesmengo left a comment

Choose a reason for hiding this comment

frandiox commented Aug 22, 2024

frandiox Aug 22, 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

jamesmengo Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmengo left a comment • edited Loading

Choose a reason for hiding this comment

frandiox commented Aug 23, 2024

karreiro left a comment

Choose a reason for hiding this comment

karreiro left a comment

Choose a reason for hiding this comment

frandiox commented Aug 21, 2024 •

edited

Loading

github-actions bot commented Aug 21, 2024 •

edited

Loading

frandiox Aug 22, 2024 •

edited

Loading

jamesmengo Aug 22, 2024 •

edited

Loading

jamesmengo left a comment •

edited

Loading