Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import {ApplicationToken, IdentityToken} from '../../session/schema.js'
import {ExchangeScopes} from '../../session/exchange.js'
import {API} from '../../api.js'

export 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 {
return ''
}

applicationId(_api: API): string {
return ''
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import {IdentityClient} from './identity-client.js'
import {ApplicationToken, IdentityToken} from '../../session/schema.js'
import {ExchangeScopes} from '../../session/exchange.js'

export class IdentityMockClient extends IdentityClient {
async requestAccessToken(_scopes: string[]): Promise<IdentityToken> {
return {} as IdentityToken
}

async exchangeAccessForApplicationTokens(
_identityToken: IdentityToken,
_scopes: ExchangeScopes,
_store?: string,
): Promise<{[x: string]: ApplicationToken}> {
return {}
}

async refreshAccessToken(_currentToken: IdentityToken): Promise<IdentityToken> {
return {} as IdentityToken
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import {IdentityClient} from './identity-client.js'
import {ApplicationToken, IdentityToken} from '../../session/schema.js'
import {ExchangeScopes} from '../../session/exchange.js'

export class IdentityServiceClient extends IdentityClient {
async requestAccessToken(_scopes: string[]): Promise<IdentityToken> {
return {} as IdentityToken
}

async exchangeAccessForApplicationTokens(
_identityToken: IdentityToken,
_scopes: ExchangeScopes,
_store?: string,
): Promise<{[x: string]: ApplicationToken}> {
return {}
}

async refreshAccessToken(_currentToken: IdentityToken): Promise<IdentityToken> {
return {} as IdentityToken
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import {Environment} from '../../context/service.js'
import {describe, expect, test, vi, beforeEach} from 'vitest'

const mockServiceEnvironment = vi.fn()
const mockIsRunning2024 = vi.fn()

vi.mock('../../context/service.js', async () => {
const actual = await vi.importActual('../../context/service.js')
return {
...actual,
serviceEnvironment: (...args: unknown[]) => mockServiceEnvironment(...args),
}
})

vi.mock('../../../../public/node/vendor/dev_server/dev-server-2024.js', async () => {
const actual = await vi.importActual('../../../../public/node/vendor/dev_server/dev-server-2024.js')
return {
...actual,
isRunning2024: (...args: unknown[]) => mockIsRunning2024(...args),
}
})

describe('getIdentityClient', () => {
beforeEach(async () => {
mockServiceEnvironment.mockReset()
mockIsRunning2024.mockReset()
vi.resetModules()
})

test('returns IdentityServiceClient when environment is Production', async () => {
mockServiceEnvironment.mockReturnValue(Environment.Production)
mockIsRunning2024.mockReturnValue(false)

const {getIdentityClient} = await import('./instance.js')

const instance = getIdentityClient()

expect(instance.constructor.name).toBe('IdentityServiceClient')
})

test('returns IdentityServiceClient when environment is Local and identity service is running', async () => {
mockServiceEnvironment.mockReturnValue(Environment.Local)
mockIsRunning2024.mockReturnValue(true)

const {getIdentityClient} = await import('./instance.js')

const instance = getIdentityClient()

expect(instance.constructor.name).toBe('IdentityServiceClient')
expect(mockIsRunning2024).toHaveBeenCalledWith('identity')
})

test('returns IdentityMockClient when environment is Local and identity service is not running', async () => {
mockServiceEnvironment.mockReturnValue(Environment.Local)
mockIsRunning2024.mockReturnValue(false)

const {getIdentityClient} = await import('./instance.js')

const instance = getIdentityClient()

expect(instance.constructor.name).toBe('IdentityMockClient')
expect(mockIsRunning2024).toHaveBeenCalledWith('identity')
})

test('returns the same instance on subsequent calls (singleton pattern)', async () => {
mockServiceEnvironment.mockReturnValue(Environment.Production)
mockIsRunning2024.mockReturnValue(false)

const {getIdentityClient} = await import('./instance.js')

const firstInstance = getIdentityClient()
const secondInstance = getIdentityClient()

expect(firstInstance).toBe(secondInstance)
expect(mockServiceEnvironment).toHaveBeenCalledTimes(1)
})
})
18 changes: 18 additions & 0 deletions packages/cli-kit/src/private/node/clients/identity/instance.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import {IdentityClient} from './identity-client.js'
import {IdentityMockClient} from './identity-mock-client.js'
import {IdentityServiceClient} from './identity-service-client.js'
import {Environment, serviceEnvironment} from '../../context/service.js'
import {isRunning2024} from '../../../../public/node/vendor/dev_server/dev-server-2024.js'

let _identityClient: IdentityClient | undefined

export function getIdentityClient() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think is OK to have this here 👌

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you ever access the undefined variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

_identityClient = client
}

return _identityClient
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,15 @@ function assertRunning2024(projectName: string): void {
})
}

export function isRunning2024(projectName: string) {
try {
assertRunning2024(projectName)
return true
} catch (_) {
return false
}
}

function getBackendIp(projectName: string): string {
try {
const backendIp = resolveBackendHost(projectName)
Expand Down