-
Notifications
You must be signed in to change notification settings - Fork 290
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
Add login
and logout
commands
#1022
Conversation
This comment has been minimized.
This comment has been minimized.
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.
Everything in here is looking great! I'm running into some errors when testing out the branch due to a platform error that we're fixing. Once that's resolved I'll come back and finish testing!
[ | ||
'Run', | ||
{command: `${cliCommand} link`}, | ||
'to link your store to this project.', |
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.
'to link your store to this project.', | |
'to link a Hydrogen storefront to this project.', |
maybe clearer?
@@ -21,14 +21,6 @@ export const commonFlags = { | |||
env: 'SHOPIFY_HYDROGEN_FLAG_FORCE', | |||
char: 'f', | |||
}), | |||
shop: Flags.string({ |
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.
🎉 so much better
message: outputContent`Run ${outputToken.genericShellCommand( | ||
`npx shopify hydrogen link`, | ||
)}?`.value, | ||
message: ['Run', {command: `${cliCommand} link`}, '?'], |
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.
thanks for fixing this formatting ❤️
[ | ||
([first]) => | ||
typeof first === 'string' && | ||
(first.includes('Shopify Partners') || first.includes('Logged in')), |
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.
It showing "Shopify Partners" has annoyed me, too. I wonder if we should fix this upstream in cli-kit
.
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.
Yeah, it's just that it's hardcoded in a deep function so I think it would be hard for them to make it flexible -- I assume "Shopify Partners" is correct for most of the other CLIs 🤔
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.
Was able to test this on a different shop. Everything looks good to me!
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.
One little thing that I'd love to get confirmation on but otherwise the changes look good.
* Add login and logout commands * Refactor graphql client and requests, add unit tests * Update shopify-config methods and tests * Update renderErrors to use cli Command * Rework env-pull to call login and remove silent param * Rework link command to call login and remove --shop flag * Update list command to call login and remove --shop flag * Update env-list command to call login and remove --shop flag * Oclif manifest * Cleanup * Rework log replacer * Mute cli-kit auth logs * Fix mock * Merge main branch * Fix tests * Minor changes * Update package-lock * Skip writing config when no directory is passed to login * Improve unit tests * Changesets
* Add login and logout commands * Refactor graphql client and requests, add unit tests * Update shopify-config methods and tests * Update renderErrors to use cli Command * Rework env-pull to call login and remove silent param * Rework link command to call login and remove --shop flag * Update list command to call login and remove --shop flag * Update env-list command to call login and remove --shop flag * Oclif manifest * Cleanup * Rework log replacer * Mute cli-kit auth logs * Fix mock * Merge main branch * Fix tests * Minor changes * Update package-lock * Skip writing config when no directory is passed to login * Improve unit tests * Changesets
Closes https://github.com/Shopify/core-issues/issues/56397
WHY are these changes introduced?
We are refactoring how CLI login works. For that, we are introducing
h2 login
andh2 logout
, as well as removing--shop
flags from certain commands.WHAT is this pull request doing?
h2 login
andh2 logout
commands.--shop
flag from other commands.getCliCommand
where possible.HOW to test your changes?
Play with different permutations of the related commands:
When not logged in, they should prompt to login. When not linked, they should ask to run
h2 link
(env list/pull).💡 I think it would be better to check the code commit by commit.
❓ Should we mark this as a major bump for the CLI package? It's deprecating flags we just introduced in the last release.
Checklist