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

Use app context in functions #4616

Merged
merged 5 commits into from
Oct 10, 2024
Merged
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
3 changes: 2 additions & 1 deletion packages/app/src/cli/commands/app/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ export default class Build extends AppCommand {
directory: flags.path,
clientId: flags['client-id'],
forceRelink: false,
configName: flags.config,
userProvidedConfigName: flags.config,
mode: 'report',
})

await build({app, skipDependenciesInstallation: flags['skip-dependencies-installation'], apiKey})
Expand Down
3 changes: 2 additions & 1 deletion packages/app/src/cli/commands/app/function/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@ export default class FunctionBuild extends AppCommand {

public async run(): Promise<AppCommandOutput> {
const {flags} = await this.parse(FunctionBuild)

const app = await inFunctionContext({
path: flags.path,
userProvidedConfigName: flags.config,
callback: async (app, ourFunction) => {
callback: async (app, _, ourFunction) => {
await buildFunctionExtension(ourFunction, {
app,
stdout: process.stdout,
Expand Down
4 changes: 2 additions & 2 deletions packages/app/src/cli/commands/app/function/replay.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,12 @@ export default class FunctionReplay extends AppCommand {

const app = await inFunctionContext({
path: flags.path,
apiKey,
userProvidedConfigName: flags.config,
callback: async (app, ourFunction) => {
callback: async (app, _, ourFunction) => {
await replay({
app,
extension: ourFunction,
apiKey,
path: flags.path,
log: flags.log,
json: flags.json,
Expand Down
4 changes: 2 additions & 2 deletions packages/app/src/cli/commands/app/function/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export default class FunctionRun extends AppCommand {
const app = await inFunctionContext({
path: flags.path,
userProvidedConfigName: flags.config,
callback: async (app, ourFunction) => {
callback: async (app, developerPlatformClient, ourFunction) => {
let functionExport = DEFAULT_FUNCTION_EXPORT

if (flags.export !== undefined) {
Expand Down Expand Up @@ -80,7 +80,7 @@ export default class FunctionRun extends AppCommand {

const inputQueryPath = ourFunction?.configuration.targeting?.[0]?.input_query
const queryPath = inputQueryPath && `${ourFunction?.directory}/${inputQueryPath}`
const schemaPath = await getOrGenerateSchemaPath(ourFunction, app)
const schemaPath = await getOrGenerateSchemaPath(ourFunction, app, developerPlatformClient)

await runFunction({
functionExtension: ourFunction,
Expand Down
5 changes: 3 additions & 2 deletions packages/app/src/cli/commands/app/function/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,13 @@ export default class FetchSchema extends AppCommand {

const app = await inFunctionContext({
path: flags.path,
apiKey,
userProvidedConfigName: flags.config,
callback: async (app, ourFunction) => {
callback: async (app, developerPlatformClient, ourFunction) => {
await generateSchemaService({
app,
extension: ourFunction,
apiKey,
developerPlatformClient,
stdout: flags.stdout,
path: flags.path,
})
Expand Down
2 changes: 1 addition & 1 deletion packages/app/src/cli/commands/app/function/typegen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export default class FunctionTypegen extends AppCommand {
const app = await inFunctionContext({
path: flags.path,
userProvidedConfigName: flags.config,
callback: async (app, ourFunction) => {
callback: async (app, _, ourFunction) => {
await buildGraphqlTypes(ourFunction, {stdout: process.stdout, stderr: process.stderr, app})
renderSuccess({headline: 'GraphQL types generated successfully.'})
return app
Expand Down
5 changes: 5 additions & 0 deletions packages/app/src/cli/models/app/app.test-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
AppConfigurationSchema,
AppConfigurationWithoutPath,
AppInterface,
AppLinkedInterface,
CurrentAppConfiguration,
LegacyAppConfiguration,
WebType,
Expand Down Expand Up @@ -122,6 +123,10 @@ export function testApp(app: Partial<AppInterface> = {}, schemaType: 'current' |
return newApp
}

export function testAppLinked(app: Partial<AppInterface> = {}): AppLinkedInterface {
return testApp(app, 'current') as AppLinkedInterface
}

interface TestAppWithConfigOptions {
app?: Partial<AppInterface>
config: object
Expand Down
4 changes: 3 additions & 1 deletion packages/app/src/cli/models/app/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {ensurePathStartsWithSlash} from './validation/common.js'
import {ExtensionInstance} from '../extensions/extension-instance.js'
import {isType} from '../../utilities/types.js'
import {FunctionConfigType} from '../extensions/specifications/function.js'
import {ExtensionSpecification} from '../extensions/specification.js'
import {ExtensionSpecification, RemoteAwareExtensionSpecification} from '../extensions/specification.js'
import {AppConfigurationUsedByCli} from '../extensions/specifications/types/app_config.js'
import {EditorExtensionCollectionType} from '../extensions/specifications/editor_extension_collection.js'
import {UIExtensionSchema} from '../extensions/specifications/ui_extension.js'
Expand Down Expand Up @@ -226,6 +226,8 @@ export interface AppConfigurationInterface<
remoteFlags: Flag[]
}

export type AppLinkedInterface = AppInterface<CurrentAppConfiguration, RemoteAwareExtensionSpecification>

export interface AppInterface<
TConfig extends AppConfiguration = AppConfiguration,
TModuleSpec extends ExtensionSpecification = ExtensionSpecification,
Expand Down
22 changes: 17 additions & 5 deletions packages/app/src/cli/services/app-context.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,9 @@ describe('linkedAppContext', () => {
const result = await linkedAppContext({
directory: tmp,
forceRelink: false,
configName: undefined,
userProvidedConfigName: undefined,
clientId: undefined,
mode: 'report',
})

// Then
Expand All @@ -58,6 +59,7 @@ describe('linkedAppContext', () => {
}),
remoteApp: mockRemoteApp,
developerPlatformClient: expect.any(Object),
specifications: [],
})
expect(link).not.toHaveBeenCalled()
})
Expand Down Expand Up @@ -91,15 +93,17 @@ describe('linkedAppContext', () => {
const result = await linkedAppContext({
directory: tmp,
forceRelink: false,
configName: undefined,
userProvidedConfigName: undefined,
clientId: undefined,
mode: 'report',
})

// Then
expect(result).toEqual({
app: expect.any(Object),
remoteApp: mockRemoteApp,
developerPlatformClient: expect.any(Object),
specifications: [],
})
expect(link).toHaveBeenCalledWith({directory: tmp, apiKey: undefined, configName: undefined})
})
Expand All @@ -119,7 +123,13 @@ describe('linkedAppContext', () => {
})

// When
await linkedAppContext({directory: tmp, forceRelink: false, configName: undefined, clientId: undefined})
await linkedAppContext({
directory: tmp,
forceRelink: false,
userProvidedConfigName: undefined,
clientId: undefined,
mode: 'report',
})
const result = localStorage.getCachedAppInfo(tmp)

// Then
Expand Down Expand Up @@ -148,7 +158,8 @@ describe('linkedAppContext', () => {
directory: tmp,
clientId: newClientId,
forceRelink: false,
configName: undefined,
userProvidedConfigName: undefined,
mode: 'report',
})

// Then
Expand Down Expand Up @@ -187,8 +198,9 @@ describe('linkedAppContext', () => {
await linkedAppContext({
directory: tmp,
forceRelink: true,
configName: undefined,
userProvidedConfigName: undefined,
clientId: undefined,
mode: 'report',
})

// Then
Expand Down
29 changes: 17 additions & 12 deletions packages/app/src/cli/services/app-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,31 +2,35 @@ import {appFromId} from './context.js'
import {getCachedAppInfo, setCachedAppInfo} from './local-storage.js'
import {fetchSpecifications} from './generate/fetch-extension-specifications.js'
import link from './app/config/link.js'
import {AppInterface, CurrentAppConfiguration} from '../models/app/app.js'
import {OrganizationApp} from '../models/organization.js'
import {DeveloperPlatformClient, selectDeveloperPlatformClient} from '../utilities/developer-platform-client.js'
import {getAppConfigurationState, loadAppUsingConfigurationState} from '../models/app/loader.js'
import {AppLoaderMode, getAppConfigurationState, loadAppUsingConfigurationState} from '../models/app/loader.js'
import {RemoteAwareExtensionSpecification} from '../models/extensions/specification.js'
import {AppLinkedInterface} from '../models/app/app.js'

interface LoadedAppContextOutput {
app: AppInterface<CurrentAppConfiguration, RemoteAwareExtensionSpecification>
app: AppLinkedInterface
remoteApp: OrganizationApp
developerPlatformClient: DeveloperPlatformClient
specifications: RemoteAwareExtensionSpecification[]
}

/**
* Input options for the `linkedAppContext` function.
*
* @param directory - The directory containing the app.
* @param clientId - The client ID to use when linking the app or when fetching the remote app.
* @param forceRelink - Whether to force a relink of the app, this includes re-selecting the remote org and app.
* @param configName - The name of an existing config file in the app, if not provided, the cached/default one will be used.
* @param clientId - The client ID to use when linking the app or when fetching the remote app.
* @param userProvidedConfigName - The name of an existing config file in the app, if not provided, the cached/default one will be used.
* @param mode - The mode of the app loader, it can be 'strict' or 'report'. 'report' will not throw an error when the app/extension configuration is invalid.
* It is recommended to always use 'strict' mode unless the command can work with invalid configurations (like app info).
*/
interface LoadedAppContextOptions {
directory: string
forceRelink: boolean
clientId: string | undefined
configName: string | undefined
userProvidedConfigName: string | undefined
mode: AppLoaderMode
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding a default value? And could you document it in the comment above?

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 wanted to make all options mandatory and explicit, so that each command is forced to specify how it wants to load the app, i'll add some docs to this

Copy link
Contributor

Choose a reason for hiding this comment

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

If strict is the recommended and most common way, I'd prefer to make it default to simplify, but not a strong opinion.

}

/**
Expand All @@ -35,21 +39,22 @@ interface LoadedAppContextOptions {
* You can use a custom configName to load a specific config file.
* In any case, if the selected config file is not linked, this function will force a link.
*
* @returns The local app, the remote app, and the developer platform client.
* @returns The local app, the remote app, the correct developer platform client, and the remote specifications list.
*/
export async function linkedAppContext({
directory,
clientId,
forceRelink,
configName,
userProvidedConfigName,
mode,
}: LoadedAppContextOptions): Promise<LoadedAppContextOutput> {
// Get current app configuration state
let configState = await getAppConfigurationState(directory, configName)
let configState = await getAppConfigurationState(directory, userProvidedConfigName)
let remoteApp: OrganizationApp | undefined

// If the app is not linked, force a link.
if (configState.state === 'template-only' || forceRelink) {
const result = await link({directory, apiKey: clientId, configName})
const result = await link({directory, apiKey: clientId, configName: userProvidedConfigName})
remoteApp = result.remoteApp
configState = result.state
}
Expand Down Expand Up @@ -77,7 +82,7 @@ export async function linkedAppContext({
const localApp = await loadAppUsingConfigurationState(configState, {
specifications,
remoteFlags: remoteApp.flags,
mode: 'strict',
mode,
})

// If the remoteApp is the same as the linked one, update the cached info.
Expand All @@ -87,5 +92,5 @@ export async function linkedAppContext({
setCachedAppInfo({appId: remoteApp.apiKey, title: remoteApp.title, directory, orgId: remoteApp.organizationId})
}

return {app: localApp, remoteApp, developerPlatformClient}
return {app: localApp, remoteApp, developerPlatformClient, specifications}
}
36 changes: 24 additions & 12 deletions packages/app/src/cli/services/function/common.test.ts
Original file line number Diff line number Diff line change
@@ -1,29 +1,40 @@
import {getOrGenerateSchemaPath, inFunctionContext} from './common.js'
import {loadApp} from '../../models/app/loader.js'
import {testApp, testFunctionExtension} from '../../models/app/app.test-data.js'
import {AppInterface} from '../../models/app/app.js'
import {
testAppLinked,
testDeveloperPlatformClient,
testFunctionExtension,
testOrganizationApp,
} from '../../models/app/app.test-data.js'
import {AppLinkedInterface} from '../../models/app/app.js'
import {ExtensionInstance} from '../../models/extensions/extension-instance.js'
import {FunctionConfigType} from '../../models/extensions/specifications/function.js'
import {generateSchemaService} from '../generate-schema.js'
import {DeveloperPlatformClient} from '../../utilities/developer-platform-client.js'
import {linkedAppContext} from '../app-context.js'
import {describe, vi, expect, beforeEach, test} from 'vitest'
import {renderAutocompletePrompt, renderFatalError} from '@shopify/cli-kit/node/ui'
import {joinPath} from '@shopify/cli-kit/node/path'
import {isTerminalInteractive} from '@shopify/cli-kit/node/context/local'
import {fileExists} from '@shopify/cli-kit/node/fs'

vi.mock('../../models/app/loader.js')
vi.mock('../app-context.js')
vi.mock('@shopify/cli-kit/node/ui')
vi.mock('@shopify/cli-kit/node/context/local')
vi.mock('@shopify/cli-kit/node/fs')
vi.mock('../generate-schema.js')

let app: AppInterface
let app: AppLinkedInterface
let ourFunction: ExtensionInstance

beforeEach(async () => {
ourFunction = await testFunctionExtension()
app = testApp({allExtensions: [ourFunction]})
vi.mocked(loadApp).mockResolvedValue(app)
app = testAppLinked({allExtensions: [ourFunction]})
vi.mocked(linkedAppContext).mockResolvedValue({
app,
remoteApp: testOrganizationApp(),
developerPlatformClient: testDeveloperPlatformClient(),
specifications: [],
})
vi.mocked(renderFatalError).mockReturnValue('')
vi.mocked(renderAutocompletePrompt).mockResolvedValue(ourFunction)
vi.mocked(isTerminalInteractive).mockReturnValue(true)
Expand Down Expand Up @@ -87,15 +98,16 @@ describe('ensure we are within a function context', () => {

describe('getOrGenerateSchemaPath', () => {
let extension: ExtensionInstance<FunctionConfigType>
let app: AppInterface

let app: AppLinkedInterface
let developerPlatformClient: DeveloperPlatformClient
beforeEach(() => {
extension = {
directory: '/path/to/function',
configuration: {},
} as ExtensionInstance<FunctionConfigType>

app = {} as AppInterface
app = testAppLinked()
developerPlatformClient = testDeveloperPlatformClient()
})

test('returns the path if the schema file exists', async () => {
Expand All @@ -104,7 +116,7 @@ describe('getOrGenerateSchemaPath', () => {
vi.mocked(fileExists).mockResolvedValue(true)

// When
const result = await getOrGenerateSchemaPath(extension, app)
const result = await getOrGenerateSchemaPath(extension, app, developerPlatformClient)

// Then
expect(result).toBe(expectedPath)
Expand All @@ -119,7 +131,7 @@ describe('getOrGenerateSchemaPath', () => {
vi.mocked(fileExists).mockResolvedValueOnce(true)

// When
const result = await getOrGenerateSchemaPath(extension, app)
const result = await getOrGenerateSchemaPath(extension, app, developerPlatformClient)

// Then
expect(result).toBe(expectedPath)
Expand Down
Loading
Loading