Skip to content
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 option to create storefront in h2 link CLI command #1013

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

aswamy
Copy link
Contributor

@aswamy aswamy commented Jun 20, 2023

NOTE: error showing when an app isn't installed is in this PR: https://github.com/Shopify/core-issues/issues/57139

WHY are these changes introduced?

  • Adds the ability to create a storefront on Admin by doing h2 link
    • The first choice when linking a storefront is to create a new storefront
  • The default name chosen when creating a storefront is the local storefront's directory name
    • We titleize the directory name so it's more humanized. E.g. demo-storefront will become Demo Storefront. If the directory name can't be titleized, we default to Hydrogen Storefront

WHAT is this pull request doing?

  • Adding the ability to create a new storefront on Admin and link a local storefront to it
  • Update the existing h2 link tests by splitting it up between linking existing storefront vs creating and linking a new storefront

HOW to test your changes?

  • You can run h2 init with a production copy of the h2 command
  • Checkout this PR branch and navigate to packages/cli and run
    • npm i
    • npm run build
  • From the packages/cli directory, run h2 link --path=/path/to/your/project
    • You will be provided with the following new prompts
image image

When job to create API tokens fails (this can't be resolved by the user):
image

Post-merge steps

Checklist

  • I've read the Contributing Guidelines
  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've added a changeset if this PR contains user-facing or noteworthy changes
  • I've added tests to cover my changes
  • I've added or updated the documentation

@aswamy aswamy requested review from graygilmore and frandiox June 20, 2023 17:05
@github-actions

This comment has been minimized.

Copy link
Contributor

@graygilmore graygilmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't had a chance to try it out but overall this is looking pretty good! We should get @gfscott and @mynameisadamf to have a look, too.

@frandiox frandiox changed the title Add h2 link CLI command Add option to create storefront in h2 link CLI command Jun 21, 2023
Copy link
Contributor

@frandiox frandiox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Do we want to consider titleizing the text so if directory is named demo-storefront, the default name given is Demo Storefront?

I think that's a good idea 👍


Visually I'm not sure if these options are different enough 🤔
Not blocking tho, the visual department can wait.

image

Maybe more of a question for @mynameisadamf

We can apply styles on 1 single choice but I think they don't look very nice (and the number on the left doesn't change):

image image

@aswamy
Copy link
Contributor Author

aswamy commented Jun 21, 2023

Force push update:

  • fix bug in waitForJob where reject was never hit
  • user-errors lib now uses AbortError instead of renderWarning
  • directory.ts will help generated a default storefront name on Admin based on user's project's directory
  • minor clean up for tests in link.test.ts

Copy link
Contributor

@graygilmore graygilmore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot one more question: will we be handling the scenario where the app is not installed in another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - but i also want to discuss the flow on slack since we aren't using the create method anymore.

@aswamy aswamy force-pushed the add-cli-link-new branch from a8fa8a3 to 97f2de5 Compare June 23, 2023 07:37
@aswamy aswamy merged commit b80f315 into 2023-04 Jun 23, 2023
@aswamy aswamy deleted the add-cli-link-new branch June 23, 2023 08:21
FrcPpe pushed a commit to FrcPpe/hydrogen that referenced this pull request Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants