Allow --development flag to create a new theme#6657
Allow --development flag to create a new theme#6657graygilmore wants to merge 4 commits intomainfrom
--development flag to create a new theme#6657Conversation
graygilmore
force-pushed
the
gg-push-with-name
branch
from
9a88cb6 to
367cc05
Compare
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3734 tests passing in 1445 suites. Report generated by 🧪jest coverage report action from d9a6149 |
|
This PR seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action. |
graygilmore
force-pushed
the
gg-push-with-name
branch
from
367cc05 to
c0dd1ba
Compare
graygilmore
force-pushed
the
gg-push-with-name
branch
2 times, most recently
from
8b81097 to
c24eece
Compare
graygilmore
changed the title
--name flag to theme push command--development-context flag to theme push command
graygilmore
force-pushed
the
gg-push-with-name
branch
4 times, most recently
from
baa67ff to
8a26885
Compare
|
/snapit |
|
🫰✨ Thanks @graygilmore! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260203000234Caution After installing, validate the version by running just |
graygilmore
force-pushed
the
gg-push-with-name
branch
from
8a26885 to
825b91e
Compare
|
Hey @graygilmore This looks like excellent progress towards the issue I raised on the dev forums, nice one! I haven't had a chance to test the snapshotted version yet, but, some questions/notes:
Thanks again and I hope this is useful! Really keen to see this go live so we can upgrade our CI process and streamline review. |
aswamy
left a comment
aswamy
left a comment
There was a problem hiding this comment.
Overall looks like a fair change! Couple comments.
These changes can be done in a separate PR if its too heavy:
1. What do you think about updating theme list so we can identify which development theme is mine. I created all of the development themes in the image, but only the latest one says "[yours]"
2. When we do theme dev now, it will always select the latest development theme in your list. Should it try to pick the first Development (*) theme since it was created via theme dev? I dont have a strong opinion on it since you can always do theme dev -t <theme-id> but just wondering what you think. Having multiple development themes was always a little funky in the CLI.
graygilmore
force-pushed
the
gg-push-with-name
branch
2 times, most recently
from
c47c744 to
c781447
Compare
graygilmore
changed the title
--development-context flag to theme push command--development flag to create a new theme
There was a problem hiding this comment.
👋🏻 @binarymonkey84! Going to respond here so we can thread replies
Maybe update the relevant docs to suggest commit ID?
This is fully up to theme developers to decide how they want to leverage this. Maybe we could add it as an example of configuring a GitHub action, though!
Can this be added to the tests?
This is already working! The additional functionality I've added doesn't have any effect on the --json flag so this should continue working as normal.
It would be good to add clarity on how long the dev context theme will live for (before it is auto-deleted by Shopify) when it is used in this way. [...] Can the docs be updated to communicate how and when these types of dev themes get deleted / how long they persist for and any rules around that?
I'm not sure if we've ever shared the exact timing for this. I'll sync with the team to see if we can get some official documentation around this. Keep in mind, too, that there is a limit to the amount of development themes you can have (also undocumented, we'll see about having official guidelines on that, too).
At the moment, the docs say dev themes are deleted when running 'shopify auth logout'.
This might have been true at some point but is not true now. We'll get this cleaned up.
How will this work in a CI context, when we'll be using a theme access password?
In CI you'll be running the same command but also passing the --password flag. Perhaps I'm misunderstanding? Or is this in relation to the confusing wording about when development themes are deleted?
There was a problem hiding this comment.
Hey @graygilmore
Maybe update the relevant docs to suggest commit ID?
This is fully up to theme developers to decide how they want to leverage this. Maybe we could add it as an example of configuring a GitHub action, though!
Yeah, I guess it's just because in the PR there were already opinions on how devs could name their theme (think it was pull request name and something else), so I thought if we're making suggestions, then suggesting commit ID might be worth doing since it encourages using this for theme instances that are more likely to correspond to a PR at a specific commit. Not too fussed on this though!
This is already working! The additional functionality I've added doesn't have any effect on the --json flag so this should continue working as normal.
Great! Wasn't sure if the --json flag was only supported with certain flags or contexts. Given this will be probably used by developers in conjunction with the command, makes sense to check it does work but if it's guaranteed to work then all good.
It would be good to add clarity on how long the dev context theme will live for (before it is auto-deleted by Shopify) when it is used in this way. [...] Can the docs be updated to communicate how and when these types of dev themes get deleted / how long they persist for and any rules around that?
I'm not sure if we've ever shared the exact timing for this. I'll sync with the team to see if we can get some official documentation around this. Keep in mind, too, that there is a limit to the amount of development themes you can have (also undocumented, we'll see about having official guidelines on that, too).
At the moment, the docs say dev themes are deleted when running 'shopify auth logout'.
This might have been true at some point but is not true now. We'll get this cleaned up.
Amazing, yes having that clarity would really help to plan and set expectations.
How will this work in a CI context, when we'll be using a theme access password?
In CI you'll be running the same command but also passing the --password flag. Perhaps I'm misunderstanding? Or is this in relation to the confusing wording about when development themes are deleted?
It's just in relation to the confusing wording :) As in, if themes are deleted on 'shopify auth logout', but that CLI command won't be run in the CI action, it wasn't clear when the theme would actually be deleted.
Cheers for all your work on this!
graygilmore
force-pushed
the
gg-push-with-name
branch
from
c781447 to
840f446
Compare
|
/snapit |
|
🫰✨ Thanks @aswamy! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260205141355Caution After installing, validate the version by running just |
aswamy
left a comment
aswamy
left a comment
There was a problem hiding this comment.
I think oclif needs to be regenerated or something because i can't pass --development flag without args anymore
| development: Flags.custom<true | string>({ | ||
| char: 'd', | ||
| description: 'Push theme files from your remote development theme.', | ||
| description: | ||
| 'Push theme files from your remote development theme. Optionally specify a context name (e.g., PR number, branch name) to reuse or create a named development theme.', | ||
| env: 'SHOPIFY_FLAG_DEVELOPMENT', | ||
| }), | ||
| parse: async (input: string | undefined) => { | ||
| if (input === undefined) { | ||
| return true | ||
| } | ||
|
|
||
| return input | ||
| }, | ||
| })(), |
There was a problem hiding this comment.
Instead of having a custom type, could we just change it to a string?
Logic:
-
when flag is set && value == "" => development theme with no custom name
-
when flag is set && value.length > 0 => development theme with a custom name
Not sure why it's bugging out when i tophat it, but it's currently saying --development flag needs a value. Maybe oclif needs to be regenerated?
There was a problem hiding this comment.
After all that it looks like you can't have a flag that optionally accepts a value 😭
Flag options are non-positional arguments passed to the command. Flags can either be option flags which take an argument, or boolean flags which do not. An option flag must have an argument.
There was a problem hiding this comment.
im sad you had to do so much work for this :( hoping it was mostly claude 🫣
Adds a new flag called --development-context on theme push that will allow developers to manually generate new development themes. Particularly helpful in CI environments.
graygilmore
force-pushed
the
gg-push-with-name
branch
from
840f446 to
d9a6149
Compare
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 declarationspackages/cli-kit/dist/cli/api/graphql/admin/generated/find_development_theme_by_name.d.tsimport * as Types from './types.js';
import { TypedDocumentNode as DocumentNode } from '@graphql-typed-document-node/core';
export type FindDevelopmentThemeByNameQueryVariables = Types.Exact<{
name: Types.Scalars['String']['input'];
}>;
export type FindDevelopmentThemeByNameQuery = {
themes?: {
nodes: {
id: string;
name: string;
role: Types.ThemeRole;
processing: boolean;
}[];
} | null;
};
export declare const FindDevelopmentThemeByName: DocumentNode<FindDevelopmentThemeByNameQuery, FindDevelopmentThemeByNameQueryVariables>;
Existing type declarationspackages/cli-kit/dist/public/node/themes/api.d.ts@@ -5,6 +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 findDevelopmentThemeByName(name: string, 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[]>;
packages/cli-kit/dist/public/node/themes/theme-manager.d.ts@@ -8,8 +8,8 @@ export declare abstract class ThemeManager {
protected abstract removeTheme(): void;
protected abstract context: string;
constructor(adminSession: AdminSession);
- findOrCreate(): Promise<Theme>;
- fetch(): Promise<Theme | undefined>;
+ findOrCreate(name?: string, role?: Role): Promise<Theme>;
+ fetch(name?: string, role?: Role): Promise<Theme | undefined>;
generateThemeName(context: string): string;
create(themeRole?: Role, themeName?: string): Promise<Theme>;
}
\ No newline at end of file
|
|
/snapit |
|
🫰✨ Thanks @graygilmore! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260205225255Caution After installing, validate the version by running just |
3 participants
WHY are these changes introduced?
Developers often want to create a new development theme on a per-PR basis when reviewing theme changes. This is difficult right now because our systems try to only have one development theme per user which means that GitHub's CI is considered a single user and development themes get overwritten all the time.
WHAT is this pull request doing?
This PR solves this by allowing developers to provide a context by using the
--development-contextflag. The context in this case is just a name but we refer to it as a context because it's specific to development themes and we don't want to confuse with the--themeflag which accepts both an ID and a name.When a user provides
--development-contextwe'll search for all development themes with that name. If we find one we'll push to it otherwise we'll create a new development theme. This lets developers decide whether to keep long running contexts that get overwritten (e.g.PR #<number>) or new development themes on every push (e.g.PR #<number> - <latest_sha>).How to test your changes?
npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/cli@0.0.0-snapshot-20260203000234shopify theme list --role developmentto see all of your development themesshopify theme push -d -c test-1shopify theme list --role developmentagain and ensure you see the new themeshopify theme push -d -c test-2shopify theme list --role developmentagain and ensure a new theme is created (ie, the previous development theme isn't replaced)shopify theme push -d -c test-1againshopify theme list --role developmentagain and ensure test-1 was replaced and there aren't two themes named test-1