Repository navigation
Allow theme push and share commands to be called with multiple environments - #6361
Conversation
Coverage report
Show files with reduced coverage 🔻
Test suite run success3323 tests passing in 1384 suites. Report generated by 🧪jest coverage report action from 47d198d |
| const environment = flags.environment | ||
|
|
||
| // when push is used programmatically, we don't have an admin session, so need to create one | ||
| const session = |
There was a problem hiding this comment.
| return true | ||
| } | ||
|
|
||
| if (multiEnvironment) { |
There was a problem hiding this comment.
Since multi env commands are non-interactive, trying to push to a live theme without allow-live will prevent uploads and error
| export type RequiredFlags = (string | string[])[] | null | ||
|
|
||
| export default abstract class ThemeCommand extends Command { | ||
| passThroughFlags(flags: FlagValues, {allowedFlags}: PassThroughFlagsOptions): string[] { |
There was a problem hiding this comment.
I believe this it leftover from CLI2, it was used in pull/push/share but there doesnt seem to be any references to it now
There was a problem hiding this comment.
Right, those were flags we passed to the CLI2 when we used it internally during the migration.
|
/snapit |
2f6709a to
e873ee1
Compare
|
/snapit |
| /** | ||
| * Initiates the push process based on provided flags. | ||
| * | ||
| * @param flags - The flags for the push operation. |
There was a problem hiding this comment.
I couldn't decide if it would be helpful to add the new params to the docstring - when the function is used programmatically likely only flags would be passed in
| return true | ||
| } | ||
|
|
||
| if (multiEnvironment) { |
There was a problem hiding this comment.
In multi environment commands we're returning false if a user can't confirm to align with other multi env command behaviour. However, in single/no env runs we're returning true and continuing
e873ee1 to
6d903b1
Compare
6d903b1 to
e986528
Compare
|
/snapit |
|
🫰✨ Thanks @dejmedus! 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-20250912171601Caution After installing, validate the version by running just |
e986528 to
d44bc9e
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-20250916211333Caution After installing, validate the version by running just |
graygilmore
left a comment
graygilmore
left a comment
There was a problem hiding this comment.
🎩 went well! Only one real comment here around how we render the flags. Since it's a larger change let's make sure we get another person from the team to look at it.
| onComplete?: (ctx: TContext) => void | ||
| abortSignal?: AbortSignal | ||
| noColor?: boolean | ||
| noProgressBar?: boolean |
There was a problem hiding this comment.
(nit) generally prefer positive booleans like progressBar so that logic reads a little nicer than !noX which takes the brain a little bit of work but since noColor is already there maybe it's fine to leave it.
| selectedTheme, | ||
| session, | ||
| { | ||
| path: workingDirectory, |
There was a problem hiding this comment.
(super nit) since we're already touching this, feel free to alphabetize
| const usedFlag = Array.isArray(flag) ? flag.find((flag) => flags[flag]) : flag | ||
| return usedFlag && [usedFlag.includes('password') ? usedFlag : `${usedFlag}: ${flags[usedFlag]}`] | ||
| if (usedFlag === 'password') return `password` | ||
| if (usedFlag === 'path' && typeof flags.path === 'string') return `path: ..${flags.path.slice(-12)}` |
There was a problem hiding this comment.
Since path will always be set I wonder if we should tweak this a bit? If you had a long folder name it'd be pretty cut off and you might not know what it's saying (especially since .. has special meaning in the CLI when working with paths.
Maybe we skip adding this one entirely unless it's explicitly provided?
Just in case it's not expected: I don't have path in my shopify.theme.toml for either environment I'm testing this on and I see:
➜ aurora-theme git:(main) shopify theme push -e aurora -e farm --allow-live
? Run push in the following environments?
┃ Environment
┃ • aurora store: <shop1>.com, password, path: ..aurora-theme, theme: <id>
┃ • farm store: <shop2>.myshopify.com, password, path: ..aurora-theme, theme: <id>
There was a problem hiding this comment.
Previously default flag values (like path = cwd) were being allowed to meet flag requirements. During check PR review it was decided that this shouldn't be allowed, so once that validation change merges in we will need to explicitly provide a path. I should have stacked the PRs smarter!
I agree that the truncated path looks a bit strange, could just go back to full paths or maybe it would make more sense to only show the the last / segment 🤔
There was a problem hiding this comment.
🤔 yeah I'm not sure. Full paths might be gnarly here. I like showing the last / segment but we need to make sure it's clear that we're clipping the path.
There was a problem hiding this comment.
That makes sense! I've updated it to match the screenshot: ... and the last segment. Hopefully the three dots communicate truncation well-ish, but i'm open to changing it
| const {progress: uploadProgress, promise: uploadPromise} = await uploadJobPromise | ||
|
|
||
| const timeout = options.multiEnvironment ? 4000 : 1000 | ||
| // |
`themeUploader` displays theme sync progress with an Ink animation. When multiple are ran at the same time, like in multi environment commands, it can behave unexpectedly. This commit creates alternative uploaders to prevent this
Previously, any multi environment `path` defined in `shopify.theme.toml` would not be normalized as that happened when CLI flags were parsed. This commit calls `resolvePath` when paths are loaded in from an environment
References to CLI2 have already been removed from `pull`, `push`, and `dev`. This commit removes the last remaining unused reference in `share`. `passThroughFlags` is not being used by any command so it has also been removed
When a command is run with multiple environment flags we cant prompt for confirmation. This commit allows `ensureDirectoryConfirmed` to accept environment and multiEnvironment params When a multi environment command run seems to be acting on an invalid directory an error is display and we do not confirm the action This matches the behavior of other instances where multi environment command actions cant be confirmed
Previously theme `push` and `share` commands could only be run in a single environment at a time. This commit allows them to be run in multiple environments in a single command run. Output is prefixed with the environment name, when one exists
CLI commands push and pull are exported publicly through @shopify/cli so external packages (like shopkeeper) can use them. When push is used programmatically we need to authenticate inside the function as we wont have access to the session created by ThemeCommand
d44bc9e to
2fbc81e
Compare
|
Fixed a conflict as progress bar has been split out from the |
|
/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-20250918231536Caution After installing, validate the version by running just |
| if (usedFlag === 'password') return `password` | ||
| if (usedFlag === 'path' && typeof flags.path === 'string') { | ||
| const lastSegment = flags.path.split('/').pop() || flags.path | ||
| return `path: .../${lastSegment}` |
There was a problem hiding this comment.
(this might be too much extra for this PR, feel free to ignore)
It might be best to include both the beginning and end. Something like:
~/.../folder-name
/Something/.../folder-name
What do you think? Here's some code example:
const flags = [
'single-folder',
'~/Users/graygilmore/repos/aurora-theme',
'C:\\SuperCoolDriveName\\LintingRules\\pipeline-rules\\.theme-check.yml',
'/home/runner/work/Shopify/Shopify/tools/something-else-here/cool_folder',
]
flags.forEach((flag) => {
const splits = flag.split(/[/|\\]/);
if (splits.length === 1) {
console.log(flag)
return
}
const first = splits[0] === '' ? `/${splits[1]}` : splits[0]
const last = splits.at(-1)
console.log(`${first}/.../${last}`)
})Which would output:
single-folder
~/.../aurora-theme
C:/.../.theme-check.yml
/home/.../cool_folder
I'm not 100% sure if we need to specifically handle \ for Windows here.
There was a problem hiding this comment.
I like this a lot! The only difference would be that the path has already been resolved, so relative single-folder would look something like Users/.../single-folder and same with ~
Updated: 
|
|
||
| const {progress: uploadProgress, promise: uploadPromise} = await uploadJobPromise | ||
|
|
||
| const timeout = options.multiEnvironment ? 4000 : 1000 |
There was a problem hiding this comment.
How did you pick the 4 second timeout for multi environment?
Side nit - although it was called timeout before, can we call it something like updateInterval if this is just the delay to update the progress to the terminal?
There was a problem hiding this comment.
Will do! I increased it to try to get closer to 25% jumps each time. At 1000 the logs tended to fill up shorter terminal windows, which i'm not totally against if we do want to change back.
EvilGenius13
left a comment
EvilGenius13
left a comment
There was a problem hiding this comment.
Awesome work! 🎩 went well
7acfe20 to
47d198d
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 declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/ui.d.ts@@ -322,6 +322,7 @@ interface RenderTableOptions<T extends ScalarDict> extends TableProps<T> {
export declare function renderTable<T extends ScalarDict>({ renderOptions, ...props }: RenderTableOptions<T>): string | undefined;
interface RenderTasksOptions {
renderOptions?: RenderOptions;
+ noProgressBar?: boolean;
}
/**
* Runs async tasks and displays their progress to the console.
@@ -329,7 +330,7 @@ interface RenderTasksOptions {
* ▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀▀
* Installing dependencies ...
*/
-export declare function renderTasks<TContext>(tasks: Task<TContext>[], { renderOptions }?: RenderTasksOptions): Promise<TContext>;
+export declare function renderTasks<TContext>(tasks: Task<TContext>[], { renderOptions, noProgressBar }?: RenderTasksOptions): Promise<TContext>;
/**
* Awaits a single promise and displays a loading bar while it's in progress. The promise's result is returned.
* @param options - Configuration object
packages/cli-kit/dist/private/node/ui/components/LoadingBar.d.ts@@ -2,6 +2,7 @@ import React from 'react';
interface LoadingBarProps {
title: string;
noColor?: boolean;
+ noProgressBar?: boolean;
}
-declare const LoadingBar: ({ title, noColor }: React.PropsWithChildren<LoadingBarProps>) => JSX.Element;
+declare const LoadingBar: ({ title, noColor, noProgressBar }: React.PropsWithChildren<LoadingBarProps>) => JSX.Element;
export { LoadingBar };
\ No newline at end of file
packages/cli-kit/dist/private/node/ui/components/Tasks.d.ts@@ -14,6 +14,7 @@ interface TasksProps<TContext> {
onComplete?: (ctx: TContext) => void;
abortSignal?: AbortSignal;
noColor?: boolean;
+ noProgressBar?: boolean;
}
-declare function Tasks<TContext>({ tasks, silent, onComplete, abortSignal, noColor, }: React.PropsWithChildren<TasksProps<TContext>>): JSX.Element | null;
+declare function Tasks<TContext>({ tasks, silent, onComplete, abortSignal, noColor, noProgressBar, }: React.PropsWithChildren<TasksProps<TContext>>): JSX.Element | null;
export { Tasks };
\ No newline at end of file
|
|
@dejmedus Have you considered implementing glob search, like Theme Kit has? |
|
Hi @andershagbard 👋 that makes sense! I've created an issue for the feature here |
5 participants
WHY are these changes introduced?
Previously theme
pushandshare(which is push behind the scenes) could only be run with a single--environmentflag. This PR allows them to be run with multiple environments in a single commandThis refactor was also a good opportunity to do some clean up as
sharewas the last command with leftover references to CLI2 (references have already been removed frompull,push, anddev)WHAT is this pull request doing?
pushandsharecommands in favour of ThemeCommand'srunlogicpushis exported publicly through @shopify/cli to be used by packages like shopkeeper, an auth session is created if one is not passed in fromThemeCommand(as one wouldn't be whenpushis used programatically)shopify.theme.tomlthe same way as they would be via CLIpassThroughFlagsandcli2Flagsas they are no longer usedpush
new-push-demo.mov
push as imported function
shopkeeper-demo.mov
push jq output
share
new-share-demo.mov
Allow live error
How to test your changes?
gh pr checkout 6361or install the snapit versionshopify.theme.tomlfileExample toml
pushandshareshould work as expected with no, single, and multiple--environmentflagspushshould error ifallow-liveis not provided while pushing to the live themeMeasuring impact
Checklist