-
Notifications
You must be signed in to change notification settings - Fork 125
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
[Themes][app dev][TAE] Find or Create host theme when preparing theme app extension preview #4156
Conversation
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
fce6e01
to
3119ebc
Compare
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1827 tests passing in 832 suites. Report generated by 🧪jest coverage report action from ce0342a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @jamesmengo! Great stuff 🔥 🚀 I've left only some minor comments and suggestions :)
3371bc1
to
c203335
Compare
app dev - TAE
: Find or Create host theme when preparing theme app extension preview2e5fde4
to
288fb98
Compare
We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @jamesmengo! Great stuff :) I've left only one minor suggestion about the src
parameter, and the rest of the PR looks awesome :)
05ff842
to
f8212bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @jamesmengo! I've notice a detail about the retry mechanism, please, let me know what you think about that :)
9759fc5
to
10fcba6
Compare
8f5aaf2
to
ae50381
Compare
ae50381
to
7b77c29
Compare
@karreiro I've slightly modified the flow of how we handle the |
7b77c29
to
4cb5f23
Compare
* Render progress bar when creating host theme * Render preview link after host theme is created * Update progress bar message * Add debug logs * Refactor: clean up types and arguments
Differences in type declarationsWe 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:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/themes/api.d.ts@@ -1,6 +1,6 @@
import { AdminSession } from '@shopify/cli-kit/node/session';
import { Result, Checksum, Key, Theme, ThemeAsset } from '@shopify/cli-kit/node/themes/types';
-export type ThemeParams = Partial<Pick<Theme, 'name' | 'role' | 'processing'>>;
+export type ThemeParams = Partial<Pick<Theme, 'name' | 'role' | 'processing' | 'src'>>;
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[]>;
packages/cli-kit/dist/public/node/themes/theme-manager.d.ts@@ -10,5 +10,6 @@ export declare abstract class ThemeManager {
constructor(adminSession: AdminSession);
findOrCreate(): Promise<Theme>;
fetch(): Promise<Theme | undefined>;
+ generateThemeName(context: string): string;
create(themeRole?: Role, themeName?: string): Promise<Theme>;
}
\ No newline at end of file
packages/cli-kit/dist/public/node/themes/types.d.ts@@ -60,6 +60,10 @@ export interface Theme {
* The remote role of the theme.
*/
role: string;
+ /**
+ * A public URL where Shopify can access the theme code.
+ */
+ src?: string;
}
/**
* Represents the remote checksum for a file in a theme.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @jamesmengo! The PR is in excellent shape and works as expected!
Just one minor comment, which we may apply here or in a following PR (when we introduce the other modules that handle the development server): I believe we could move the host-theme-watcher
and the host-theme-manager
to the packages/app/src/cli/utilities/extensions/theme/
directory :)
Thanks again for this PR!
WHY are these changes introduced?
note: This solution uses a placeholder URL rather than the public github URL. Further testing is required - will be tracked here
WHAT is this pull request doing?
How to test your changes?
Code.-.host-theme-manager.ts.cli.mp4
Measuring impact
How do we know this change was effective? Please choose one:
Checklist