create identity client interface #6634

alexanderMontague · 2025-11-18T20:00:09Z

WHY are these changes introduced?

Part of: https://github.com/shop/issues-develop/issues/21594

We're trying to improve our local development experience for using the CLI
A good first step is being able to hit our APIs without the identity service running

WHAT is this pull request doing?

scaffolds out the interface for the identity client that we will use
defines the client swapping capabilities based on the environment and what services are running

How to test your changes?

included tests
looking for feedback on a better singleton pattern if possible

alexanderMontague · 2025-11-18T20:00:25Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

alexanderMontague · 2025-11-18T20:05:02Z

packages/cli-kit/src/private/node/clients/identity/instance.ts

+
+let _identityClient: IdentityBaseClient | undefined
+
+export function getIdentityClient() {


I really wanted this to live in IdentityBaseClient under a getInstance method like we do for our other clients, but combining that with an abstract class leads to circular dependencies. I see we use this singleton pattern in other modules, so looking for feedback on this especially.

I think is OK to have this here 👌

github-actions · 2025-11-18T20:05:41Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	79.17% (-0.06% 🔻)	13615/17198
🟡	Branches	73.12% (+0.01% 🔼)	6640/9081
🟡	Functions	79.18% (-0.19% 🔻)	3510/4433
🟡	Lines	79.53% (-0.06% 🔻)	12860/16171

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / bulk-operation-run-query.ts	100%	100%	100%	100%
🟢	`...` / execute-bulk-operation.ts	100%	83.33%	100%	100%
🟢	`...` / run-query.ts	100%	100%	100%	100%
🔴	`...` / identity-client.ts	0%	100%	0%	0%
🔴	`...` / identity-mock-client.ts	0%	100%	0%	0%
🔴	`...` / identity-service-client.ts	0%	100%	0%	0%
🟢	`...` / instance.ts	100%	100%	100%	100%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / ConcurrentOutput.tsx	98.36% (-1.64% 🔻)	92% (-4% 🔻)	100%	98.33% (-1.67% 🔻)
🔴	`...` / ui.tsx	50.82% (-0.79% 🔻)	42.86% (-5.53% 🔻)	54.55% (+1.42% 🔼)	50% (-0.82% 🔻)
🟡	`...` / theme-environment.ts	69.57% (-1.86% 🔻)	50%	55.56% (-3.27% 🔻)	69.57% (-1.86% 🔻)

Test suite run success

3364 tests passing in 1378 suites.

Report generated by 🧪jest coverage report action from ec62db8

MitchDickinson

Some quick drive by interface feedback 😅

packages/cli-kit/src/private/node/clients/identity/identity-client.ts

packages/cli-kit/src/private/node/clients/identity/identity-base-client.ts

packages/cli-kit/src/private/node/clients/identity/identity-client.ts

packages/cli-kit/src/private/node/clients/identity/identity-base-client.ts

packages/cli-kit/src/private/node/clients/identity/identity-client.ts

ryancbahan · 2025-11-19T19:59:49Z

packages/cli-kit/src/private/node/clients/identity/instance.ts

+  if (!_identityClient) {
+    const isLocal = serviceEnvironment() === Environment.Local
+    const identityServiceRunning = isRunning2024('identity')
+    const client = isLocal && !identityServiceRunning ? new IdentityMockClient() : new IdentityServiceClient()


why not just return client and get rid of the mutable variable?

Just lends itself to the singleton pattern better. Saves re-instantiating on every call, but the clients are deterministic so we could do this too. Follows existing patterns, I don't feel too strongly about this. LMK what you think

My opinion is that the tradeoff isn't worth it -- it introduces sequencing where the variable can be undefined, which has to be accounted for in the type as well as in all processes it's called from. The perf gains on compute will be near-zero, and may never actually become concrete unless you're calling identity multiple times in the same command lifecycle but different execution context.

How would you ever access the undefined variable?

The mutable variable is private, only the getIdentityClient is public and will return a valid client always, regardless of using a singleton or instantiating a new client every time.

I think is ok, to have a singleton here. Is safe and I think there is any con about it.

If eventually we decide that this client will have logic to prevent multiple refresh tokens operations from running at the same time, then we need to ensure everyone is using the same instance. (is an example, this is controlled somewhere else now)

packages/cli-kit/src/private/node/clients/identity/identity-client.ts

github-actions · 2025-11-20T16:15:57Z

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

Some seemingly private modules might be re-exported through public modules.
If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

packages/cli-kit/dist/private/node/clients/identity/identity-client.d.ts

import { ApplicationToken, IdentityToken } from '../../session/schema.js';
import { ExchangeScopes } from '../../session/exchange.js';
import { API } from '../../api.js';
export declare abstract class IdentityClient {
    abstract requestAccessToken(scopes: string[]): Promise<IdentityToken>;
    abstract exchangeAccessForApplicationTokens(identityToken: IdentityToken, scopes: ExchangeScopes, store?: string): Promise<{
        [x: string]: ApplicationToken;
    }>;
    abstract refreshAccessToken(currentToken: IdentityToken): Promise<IdentityToken>;
    clientId(): string;
    applicationId(_api: API): string;
}

packages/cli-kit/dist/private/node/clients/identity/identity-mock-client.d.ts

import { IdentityClient } from './identity-client.js';
import { ApplicationToken, IdentityToken } from '../../session/schema.js';
import { ExchangeScopes } from '../../session/exchange.js';
export declare class IdentityMockClient extends IdentityClient {
    requestAccessToken(_scopes: string[]): Promise<IdentityToken>;
    exchangeAccessForApplicationTokens(_identityToken: IdentityToken, _scopes: ExchangeScopes, _store?: string): Promise<{
        [x: string]: ApplicationToken;
    }>;
    refreshAccessToken(_currentToken: IdentityToken): Promise<IdentityToken>;
}

packages/cli-kit/dist/private/node/clients/identity/identity-service-client.d.ts

import { IdentityClient } from './identity-client.js';
import { ApplicationToken, IdentityToken } from '../../session/schema.js';
import { ExchangeScopes } from '../../session/exchange.js';
export declare class IdentityServiceClient extends IdentityClient {
    requestAccessToken(_scopes: string[]): Promise<IdentityToken>;
    exchangeAccessForApplicationTokens(_identityToken: IdentityToken, _scopes: ExchangeScopes, _store?: string): Promise<{
        [x: string]: ApplicationToken;
    }>;
    refreshAccessToken(_currentToken: IdentityToken): Promise<IdentityToken>;
}

packages/cli-kit/dist/private/node/clients/identity/instance.d.ts

import { IdentityClient } from './identity-client.js';
export declare function getIdentityClient(): IdentityClient;

Existing type declarations

packages/cli-kit/dist/public/node/vendor/dev_server/dev-server-2024.d.ts

@@ -4,6 +4,7 @@ export declare function createServer(projectName: string): {
     url: (options?: HostOptions) => string;
 };
 declare function assertRunning2024(projectName: string): void;
+export declare function isRunning2024(projectName: string): boolean;
 declare let assertRunningOverride: typeof assertRunning2024 | undefined;
 export declare function setAssertRunning(override: typeof assertRunningOverride): void;
 export {};
\ No newline at end of file

github-actions · 2025-11-20T22:06:52Z

We detected some changes at packages/*/src 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.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

alexanderMontague commented Nov 18, 2025

View reviewed changes

alexanderMontague mentioned this pull request Nov 18, 2025

implement identity client body #6635

Open

MitchDickinson reviewed Nov 19, 2025

View reviewed changes

ryancbahan reviewed Nov 19, 2025

View reviewed changes

packages/cli-kit/src/private/node/clients/identity/identity-client.ts Show resolved Hide resolved

alexanderMontague force-pushed the 11-18-create_indentity_client_interface branch from 6ec2d7e to bd49456 Compare November 19, 2025 20:32

This was referenced Nov 19, 2025

wire up identity client to current identity callsites #6641

Open

remove shared identity code that is now in client #6642

Open

create indentity client interface

ec62db8

alexanderMontague force-pushed the 11-18-create_indentity_client_interface branch from bd49456 to ec62db8 Compare November 20, 2025 16:14

alexanderMontague marked this pull request as ready for review November 20, 2025 22:06

alexanderMontague requested a review from a team as a code owner November 20, 2025 22:06

alexanderMontague mentioned this pull request Nov 20, 2025

add option to bypass running service check if configured #6647

Open

alexanderMontague changed the title ~~create indentity client interface~~ create identity client interface Nov 20, 2025

alexanderMontague mentioned this pull request Nov 21, 2025

Refactor identity client code execution #6654

Open


		let _identityClient: IdentityBaseClient \| undefined

		export function getIdentityClient() {

create identity client interface #6634

Are you sure you want to change the base?

create identity client interface #6634

Uh oh!

Conversation

alexanderMontague commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Uh oh!

alexanderMontague commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexanderMontague Nov 18, 2025

Choose a reason for hiding this comment

Uh oh!

isaacroldan Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage report

Test suite run success

Uh oh!

MitchDickinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryancbahan Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

alexanderMontague Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

ryancbahan Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

alexanderMontague Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

isaacroldan Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Nov 20, 2025

Differences in type declarations

New type declarations

Existing type declarations

Uh oh!

github-actions bot commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexanderMontague commented Nov 18, 2025 •

edited

Loading

alexanderMontague commented Nov 18, 2025 •

edited

Loading

github-actions bot commented Nov 18, 2025 •

edited

Loading