Store auth restructure#7187
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
dmerand
force-pushed
the
dlm-store-auth-preserve-scopes
branch
from
aa2f837 to
eefef3a
Compare
dmerand
force-pushed
the
dlm-store-auth-restructure
branch
from
ec9fc5b to
176cf3b
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
dmerand
force-pushed
the
dlm-store-auth-restructure
branch
from
176cf3b to
81f77d7
Compare
| @@ -0,0 +1,132 @@ | |||
| import {normalizeStoreFqdn} from '@shopify/cli-kit/node/context/fqdn' | |||
There was a problem hiding this comment.
total nit from me: i am very against index files. they obscure intent and become a dumping ground, plus barrel files cause circular deps and tree shaking issues in many cases
| onListening?: () => void | Promise<void> | ||
| } | ||
|
|
||
| function renderAuthCallbackPage(title: string, message: string): string { |
| settleWithError(new AbortError('Timed out waiting for OAuth callback.')) | ||
| }, timeoutMs) | ||
|
|
||
| const server = createServer((req, res) => { |
There was a problem hiding this comment.
odd to create a dedicated server as a closure in a callback, no?
| settleWithError(new AbortError('Timed out waiting for OAuth callback.')) | ||
| }, timeoutMs) | ||
|
|
||
| const server = createServer((req, res) => { |
There was a problem hiding this comment.
odd to create a dedicated server as a closure in a callback, no?
ryancbahan
left a comment
ryancbahan
left a comment
There was a problem hiding this comment.
I like the direction of this pr, but the seams still seem a little unclear to me. I wonder if we can invert the order of initialization a bit -- especially in callback and pkce files -- so that we first initialize the server explicitly, initialize the persistence stores, establish the dynamic route(s) that need to be hit, and then let the callback chain(s) flow.
| success: (store: string, email?: string) => void | ||
| } | ||
|
|
||
| interface StoreAuthDependencies { |
There was a problem hiding this comment.
I don't know that the DI pattern here is particularly serving us
2 participants
What
Restructure
shopify store authunderpackages/cli/src/cli/services/store/auth/and split the main auth responsibilities into explicit auth-domain seams.This keeps the command contract the same while making auth ownership easier to review and build on.
Why
The store auth fast-follow landed quickly and left auth responsibilities spread across a few files with blurry ownership.
That made it harder to reason about:
It also made the execute-side follow-up harder to stage cleanly.
The goal here is to make the auth domain legible on its own before building the execute-side cleanup on top.
How
This PR moves auth code under
services/store/auth/and separates:auth/index.ts— auth orchestrationauth/session-store.ts— local session bucket storage and current-user selectionauth/session-lifecycle.ts— expiry / refresh / invalidation policyauth/token-client.ts— auth-related HTTPauth/config.ts/auth/recovery.ts— shared auth support codeIt also:
getCurrentStoredStoreAppSession(...)so current-user semantics are explicitHigh-level ownership after this PR:
This PR is intentionally auth-only. The execute-side folder move and full execute-context cleanup stay in the next stacked PR.
Testing
Manual checks:
pnpm run shopify store auth --store donaldmerand.myshopify.com --scopes read_products pnpm run shopify store execute --store donaldmerand.myshopify.com --query 'query { shop { name id } }' node packages/cli/bin/run.js store auth --help node packages/cli/bin/run.js store execute --helpVerify that:
store authstill authenticates and persists store auth successfullystore executestill reuses stored auth successfullyConsidered
persistence/transportfolders: rejected in favor of domain-firststore/authownership.Measuring impact
Checklist