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 tunneling in dev command #1804

Merged
merged 17 commits into from
Mar 18, 2024
Merged

add tunneling in dev command #1804

merged 17 commits into from
Mar 18, 2024

Conversation

michenly
Copy link
Contributor

@michenly michenly commented Feb 29, 2024

WHY are these changes introduced?

The first step for making developing with Customer Account API easier.

Where instead of using ngrok on top of shopify CLI, a tunnling service will come with using npx shopify dev --tunnel

Step two will take the tunnel URL and push the redirect url to Admin.

WHAT is this pull request doing?

flag --customer-account-push had been added to the CLI hydrogen dev & hydrogen dev-vite commands.

--customer-account-push flag: A cloud flare tunnel is automatically started and the url display instead of localhost. At the end up starting a tunnel, the CLI will then take the tunneling url, push it to admin.

A new command hydrogen customer-account push had been added with the following flag where it can be use as a stand alone command to push config

If the push is successful, the urls are then saved to .shopify/project.json file. And the next push will take the url here and remove it from admin while doing the next update

HOW to test your changes?

npm run dev:app

to see automatically tunnel running. The --customer-account-push is added to skeleton template along side of having new Customer Account API

There are a few flows to check:

  1. Run with mock.shop in the .env file: see the dev server running with info banner in the server log stating the flag functionalities had been skip
  2. Happy path (without mock.shop) where running hydrogen dev --customer-account-push or hydrogen dev-vite --customer-account-push => can interact with CA API in browser
  3. Or run hydrogen customer-account push --dev-oring <your-persisting-ngrok-url> as a one off to push local dev url, production url, or even preview url to admin [cc @macpham would this be something oxygen need when generating preview?]

Still To Do:

Nice to Have:

  • Maybe get something.hydrogen.app working instead of *.trycloudflare.com https://github.com/orgs/Shopify/projects/5093/views/16?pane=issue&itemId=55000036
    (Implement branded domains if it's an OK feature. Deploy a Cloudflare Worker with a custom domain like hydrogen.app and use Cloudflare's wild card subdomains to proxy the trycloudflare tunnel. For example, we check the Request in the worker and, if it was to xyz.hydrogen.app, we forward it to xyz.trycloudflare.com. Then, we show the users they can connect to xyz.hydrogen.app in the terminal.)

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

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.

Haven't tried this yet but just a few quick comments.
We probably need to modify Vite's server URLs before calling printUrls 🤔
Also, not sure how this plays with the --host flag. That flag is already turning localhost into 0.0.0.0 so that the site is available in the local network. It maps Vite's host.

packages/cli/src/commands/hydrogen/dev-vite.ts Outdated Show resolved Hide resolved
packages/cli/src/commands/hydrogen/dev-vite.ts Outdated Show resolved Hide resolved
packages/cli/package.json Outdated Show resolved Hide resolved
@github-actions github-actions bot had a problem deploying to preview March 1, 2024 15:27 Failure
@github-actions github-actions bot had a problem deploying to preview March 1, 2024 15:33 Failure
@michenly michenly force-pushed the mc-dev-tunnel-flag branch 2 times, most recently from 3a3bb7f to eb43822 Compare March 1, 2024 21:21
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.

Thanks for the updates! I've left a few things to consider in the comments.

I want to make sure I understand the flow:

  1. We are adding the flag by default, but devs can still use mock.shop because we keep that in .env by default, right?
  2. If they use mock.shop (by default they do), we only show a banner to ask them to link the storefront...?
  3. Once they have removed the mock.shop domain, we ask for login/link and start tunneling.

Is that somewhat correct? Thanks!

--

Also, I'm confused, do I still need to do a manual step? Shouldn't it be all automatic with these changes?

image

(I've chosen Hydrogen Preview).

packages/cli/package.json Outdated Show resolved Hide resolved
packages/hydrogen/src/customer/customer.ts Outdated Show resolved Hide resolved
packages/hydrogen/src/customer/customer.ts Outdated Show resolved Hide resolved
templates/skeleton/README.md Show resolved Hide resolved
packages/cli/src/commands/hydrogen/dev.ts Outdated Show resolved Hide resolved
packages/cli/src/commands/hydrogen/dev-vite.ts Outdated Show resolved Hide resolved
@michenly
Copy link
Contributor Author

michenly commented Mar 8, 2024

@frandiox

We are adding the flag by default, but devs can still use mock.shop because we keep that in .env by default, right?
Yes, but the dev server will act like if the flag is remove

If they use mock.shop (by default they do), we only show a banner to ask them to link the storefront...?
The banner state you can continue knowing CA API will fail or link shop. Good for mock.shop user who have not sign up for a shop yet

Once they have removed the mock.shop domain, we ask for login/link and start tunneling.
Correct, now the flag will start working

The error you see is a mutation failure, I have only seems it so far in core during testing when a storefront is private. Which MIGHT be what Hydrogen Preview is. Will talk to Hydrogen Admin team and sort this out.

Looks like the issue is happening because:

  1. The Hydrogen Preview has multiple storefronts and not all of them are set to public CA API.
  2. CA API seems to be broken. It looks like we have switch the storefront recently and the new one is not set up for CA API. Will get this fix up ASAP.
  3. Any new storefront created in Hydrogen App from now on will have public type, talking to the team about making old storefront public as well.

@michenly michenly requested a review from frandiox March 8, 2024 19:59
@michenly michenly force-pushed the mc-dev-tunnel-flag branch 3 times, most recently from 3d4728c to bb20fab Compare March 8, 2024 20:33
@michenly
Copy link
Contributor Author

michenly commented Mar 8, 2024

I added an error link so if the "Javascript origin is not allow on this application type occur again", the message will take you to here to enable it to public type
Screenshot 2024-03-08 at 5 02 35 PM

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.

Added a few extra comments to clarify what I meant in the previous review 👍

.changeset/red-turkeys-sing.md Outdated Show resolved Hide resolved
packages/cli/src/commands/hydrogen/dev.ts Outdated Show resolved Hide resolved
packages/cli/src/lib/dev-shared.ts Outdated Show resolved Hide resolved
@michenly michenly merged commit b3323e5 into main Mar 18, 2024
13 checks passed
@michenly michenly deleted the mc-dev-tunnel-flag branch March 18, 2024 18:39
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.

None yet

4 participants