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

Gate GraphiQL for Shopifolk only #2821

Merged
merged 4 commits into from
Sep 14, 2023
Merged

Gate GraphiQL for Shopifolk only #2821

merged 4 commits into from
Sep 14, 2023

Conversation

amcaplan
Copy link
Contributor

WHY are these changes introduced?

If a sudden need arises to ship main as a new release, we shouldn't block it just because GraphiQL is still ripening.

WHAT is this pull request doing?

For Shopifolk/spin, things proceed as before. For external parties, GraphiQL is an opt-in via env var SHOPIFY_CLI_ENABLE_GRAPHIQL_EXPLORER.

How to test your changes?

# GraphiQL Explorer option shouldn't appear, and hitting `g` should have no effect.
SHOPIFY_RUN_AS_USER=1 dev shopify app dev --path /path/to/your/app

# GraphiQL Explorer should be an option, and hitting `g` should open it correctly.
SHOPIFY_RUN_AS_USER=1 SHOPIFY_CLI_ENABLE_GRAPHIQL_EXPLORER=1 dev shopify app dev --path /path/to/your/app

# Repeat the same experiment, but adding SHOPIFY_CLI_NEW_DEV=0 so you can try with old dev as well.

Measuring impact

How do we know this change was effective? Please choose one:

  • n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
  • Existing analytics will cater for this addition
  • PR includes analytics changes to measure impact

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes
  • I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

@github-actions
Copy link
Contributor

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

  • Themes: @shopify/theme-code-tools
  • UI extensions: @shopify/ui-extensions-cli
    • Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
  • Hydrogen: @shopify/hydrogen
  • Other: @shopify/cli-foundations

@github-actions
Copy link
Contributor

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 13, 2023

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
72.95% (-0.03% 🔻)
6056/8302
🟡 Branches
69.52% (-0.21% 🔻)
2958/4255
🟡 Functions 71.79% 1545/2152
🟡 Lines
74.29% (-0.04% 🔻)
5747/7736
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🔴
... / dev.ts
13.14% (-0.23% 🔻)
6.02% (-0.44% 🔻)
16.28%
12.99% (-0.26% 🔻)
🟢
... / setup-dev-processes.ts
95.45% (+0.22% 🔼)
70.37% (-7.41% 🔻)
90%
94.87% (+0.28% 🔼)
🟢
... / Dev.tsx
95.65%
86.05% (-1.76% 🔻)
93.75% 95.52%
🟢
... / ConcurrentOutput.tsx
97.62% (-2.38% 🔻)
75% (-8.33% 🔻)
100%
97.44% (-2.56% 🔻)

Test suite run success

1432 tests passing in 671 suites.

Report generated by 🧪jest coverage report action from b0b6977

@amcaplan
Copy link
Contributor Author

I added fixes for 2 issues:

  1. It fails when there are no scopes set (error coming from the shopify API package that, reasonably, requires at least 1 scope to be set)
  2. It fails on extension-only apps because we don’t do the OAuth redirect-url overwrites, so we can't complete an auth flow

For now, in both cases, we simply shouldn't show the option to open GraphiQL.

This is necessary because we have to add an OAuth callback URL. If we
are not updating URLs, GraphiQL won't work.
@amcaplan amcaplan changed the title Gate graphiql for shopifolk Gate GraphiQL for shopifolk Sep 14, 2023
@amcaplan amcaplan changed the title Gate GraphiQL for shopifolk Gate GraphiQL for Shopifolk only Sep 14, 2023
@amcaplan amcaplan added this pull request to the merge queue Sep 14, 2023
@matteodepalo
Copy link
Contributor

Nice! Once this is in nightly I can recommend its usage with the flag 😃

Copy link
Contributor

@gonzaloriestra gonzaloriestra left a comment

Choose a reason for hiding this comment

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

Working as expected 👌

Merged via the queue into main with commit b226f2a Sep 14, 2023
1 check passed
@amcaplan amcaplan deleted the gate-graphiql-for-shopifolk branch September 14, 2023 10:36
@shopify-shipit shopify-shipit bot temporarily deployed to nightly September 14, 2023 10:44 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to production October 19, 2023 13:18 Inactive
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