-
Notifications
You must be signed in to change notification settings - Fork 150
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
Enable password flag for theme dev #953
Conversation
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
Coverage report
Test suite run success871 tests passing in 443 suites. Report generated by 🧪jest coverage report action from 3b37c67 |
@@ -357,7 +357,7 @@ export function getOutputUpdateCLIReminder( | |||
const versionMessage = `💡 Version ${version} available!` | |||
if (!packageManager || packageManager === 'unknown') return versionMessage | |||
|
|||
const updateCommand = token.packagejsonScript(packageManager, 'shopify', 'upgrade') | |||
const updateCommand = token.packagejsonScript(packageManager, 'shopify upgrade') |
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.
This was showing something like Run npm run shopify -- upgrade
for npm.
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.
If I'm not wrong I think that's necessary with npm
because otherwise it treats those flags and arguments as npm
's
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.
Not sure what you mean. In this case, upgrade
is not a flag, but part of the command, so we don't need the --
separator. The third argument is used for flags, which do need the separator for npm.
We are doing the same in other places: https://github.com/Shopify/cli/blob/upgrade-cli2/packages/cli-hydrogen/src/cli/services/preview.ts#L23
@shainaraskas could you please update the documentation here to add the password flag (like the one in the list command, for example)? 🙏 |
WHY are these changes introduced?
Related to https://github.com/Shopify/internal-cli-foundations/issues/332
Since #325, most theme commands were already working with Theme Access passwords, but the
dev theme
command required additional work. Now it's ready and we can enable the password flag for that command it as well.WHAT is this pull request doing?
theme dev
commandHow to test your changes?
pnpm shopify theme dev --store your-store --password your-password --path /your/theme/path
SHOPIFY_FLAG_STORE=your-store SHOPIFY_CLI_THEME_TOKEN=your-password pnpm shopify theme dev
Measuring impact
How do we know this change was effective? Please choose one:
Checklist
dev
ordeploy
have been reflected in the internal flowchart.