From 43f0bd5d521ce247430c1ea539dfe98cc85b1fcc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Tue, 26 Nov 2024 12:29:35 +0100 Subject: [PATCH 1/4] Cache json schema validators --- packages/app/src/cli/utilities/json-schema.ts | 3 ++- packages/cli-kit/src/public/node/json-schema.ts | 12 ++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/app/src/cli/utilities/json-schema.ts b/packages/app/src/cli/utilities/json-schema.ts index 20cbc09e40a..33f2b3bf589 100644 --- a/packages/app/src/cli/utilities/json-schema.ts +++ b/packages/app/src/cli/utilities/json-schema.ts @@ -19,6 +19,7 @@ export async function unifiedConfigurationParserFactory( return merged.parseConfigurationObject } const contract = await normaliseJsonSchema(contractJsonSchema) + const extensionIdentifier = merged.identifier const parseConfigurationObject = (config: object): ParseConfigurationResult => { // First we parse with zod. This may also change the format of the data. @@ -29,7 +30,7 @@ export async function unifiedConfigurationParserFactory( const subjectForAjv = zodValidatedData ?? (config as JsonMapType) const subjectForAjvWithoutFirstClassFields = configWithoutFirstClassFields(subjectForAjv) - const jsonSchemaParse = jsonSchemaValidate(subjectForAjvWithoutFirstClassFields, contract) + const jsonSchemaParse = jsonSchemaValidate(subjectForAjvWithoutFirstClassFields, contract, extensionIdentifier) // Finally, we de-duplicate the error set from both validations -- identical messages for identical paths are removed let errors = zodParse.errors || [] diff --git a/packages/cli-kit/src/public/node/json-schema.ts b/packages/cli-kit/src/public/node/json-schema.ts index 2c740a3550f..4ffdea4d91f 100644 --- a/packages/cli-kit/src/public/node/json-schema.ts +++ b/packages/cli-kit/src/public/node/json-schema.ts @@ -2,7 +2,7 @@ import {ParseConfigurationResult} from './schema.js' import {getPathValue} from '../common/object.js' import {capitalize} from '../common/string.js' -import {Ajv, ErrorObject, SchemaObject} from 'ajv' +import {Ajv, ErrorObject, SchemaObject, ValidateFunction} from 'ajv' import $RefParser from '@apidevtools/json-schema-ref-parser' type AjvError = ErrorObject @@ -22,6 +22,8 @@ export async function normaliseJsonSchema(schema: string): Promise return parsedSchema } +const validatorsCache = new Map() + /** * Given a subject object and a JSON schema contract, validate the subject against the contract. * @@ -29,15 +31,21 @@ export async function normaliseJsonSchema(schema: string): Promise * * @param subject - The object to validate. * @param schema - The JSON schema to validate against. + * @param identifier - The identifier of the schema being validated, used to cache the validator. * @returns The result of the validation. If the state is 'error', the errors will be in a zod-like format. */ export function jsonSchemaValidate( subject: object, schema: SchemaObject, + identifier: string, ): ParseConfigurationResult & {rawErrors?: AjvError[]} { const ajv = new Ajv({allowUnionTypes: true}) + ajv.addKeyword('x-taplo') - const validator = ajv.compile(schema) + + const validator = validatorsCache.get(identifier) ?? ajv.compile(schema) + validatorsCache.set(identifier, validator) + validator(subject) // Errors from the contract are post-processed to be more zod-like and to deal with unions better From 271acdb7ff3ab7026f7c868b912ab3164e06cadd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Tue, 26 Nov 2024 12:59:34 +0100 Subject: [PATCH 2/4] Loader improvements --- packages/app/package.json | 1 + .../app/src/cli/models/app/loader.test.ts | 5 +- packages/app/src/cli/models/app/loader.ts | 64 +++++++++++++++---- .../app-events/app-event-watcher-handler.ts | 18 ++---- .../dev/app-events/app-event-watcher.test.ts | 17 ++--- .../services/dev/app-events/file-watcher.ts | 4 +- .../dev/processes/setup-dev-processes.test.ts | 13 ++-- .../dev/processes/setup-dev-processes.ts | 2 +- .../src/public/node/json-schema.test.ts | 10 +-- pnpm-lock.yaml | 8 +++ 10 files changed, 91 insertions(+), 51 deletions(-) diff --git a/packages/app/package.json b/packages/app/package.json index b1d9c54e505..867222571ae 100644 --- a/packages/app/package.json +++ b/packages/app/package.json @@ -67,6 +67,7 @@ "graphql-request": "5.2.0", "h3": "0.7.21", "http-proxy": "1.18.1", + "ignore": "6.0.2", "proper-lockfile": "4.1.2", "react": "^18.2.0", "react-dom": "18.2.0", diff --git a/packages/app/src/cli/models/app/loader.test.ts b/packages/app/src/cli/models/app/loader.test.ts index 8525ad56684..452a54ddcfb 100644 --- a/packages/app/src/cli/models/app/loader.test.ts +++ b/packages/app/src/cli/models/app/loader.test.ts @@ -9,6 +9,7 @@ import { AppLoaderMode, getAppConfigurationState, loadConfigForAppCreation, + resetDidCheckForGlobalCLIWarning, } from './loader.js' import {LegacyAppSchema, WebConfigurationSchema} from './app.js' import {DEFAULT_CONFIG, buildVersionedAppSchema, getWebhookConfig} from './app.test-data.js' @@ -328,6 +329,7 @@ wrong = "property" test('shows warning if using global CLI but app has local dependency', async () => { // Given + resetDidCheckForGlobalCLIWarning() vi.mocked(globalCLIVersion).mockResolvedValue('3.68.0') vi.mocked(localCLIVersion).mockResolvedValue('3.65.0') const mockOutput = mockAndCaptureOutput() @@ -2353,6 +2355,7 @@ wrong = "property" devDependencies: {}, }) await writeFile(joinPath(webDirectory, 'package.json'), JSON.stringify({})) + await writeFile(joinPath(tmpDir, '.gitignore'), '') await loadTestingApp() @@ -2364,7 +2367,7 @@ wrong = "property" cmd_app_all_configs_any: true, cmd_app_all_configs_clients: JSON.stringify({'shopify.app.toml': '1234567890'}), cmd_app_linked_config_name: 'shopify.app.toml', - cmd_app_linked_config_git_tracked: false, + cmd_app_linked_config_git_tracked: true, cmd_app_linked_config_source: 'cached', cmd_app_warning_api_key_deprecation_displayed: false, app_extensions_any: false, diff --git a/packages/app/src/cli/models/app/loader.ts b/packages/app/src/cli/models/app/loader.ts index 939597d6684..5ed0a197856 100644 --- a/packages/app/src/cli/models/app/loader.ts +++ b/packages/app/src/cli/models/app/loader.ts @@ -16,6 +16,7 @@ import { BasicAppConfigurationWithoutModules, SchemaForConfig, AppCreationDefaultOptions, + AppLinkedInterface, } from './app.js' import {configurationFileNames, dotEnvFileNames} from '../../constants.js' import metadata from '../../metadata.js' @@ -47,12 +48,12 @@ import {AbortError} from '@shopify/cli-kit/node/error' import {outputContent, outputDebug, OutputMessage, outputToken} from '@shopify/cli-kit/node/output' import {joinWithAnd, slugify} from '@shopify/cli-kit/common/string' import {getArrayRejectingUndefined} from '@shopify/cli-kit/common/array' -import {checkIfIgnoredInGitRepository} from '@shopify/cli-kit/node/git' import {renderInfo} from '@shopify/cli-kit/node/ui' import {currentProcessIsGlobal} from '@shopify/cli-kit/node/is-global' import {showNotificationsIfNeeded} from '@shopify/cli-kit/node/notifications-system' import {globalCLIVersion, localCLIVersion} from '@shopify/cli-kit/node/version' import {jsonOutputEnabled} from '@shopify/cli-kit/node/environment' +import ignore from 'ignore' const defaultExtensionDirectory = 'extensions/*' @@ -203,6 +204,8 @@ export class AppErrors { interface AppLoaderConstructorArgs { mode?: AppLoaderMode loadedConfiguration: ConfigurationLoaderResult + // Used when reloading an app, to avoid some expensive steps during loading. + previousApp?: AppLinkedInterface } export async function checkFolderIsValidApp(directory: string) { @@ -252,6 +255,21 @@ export async function loadApp { + const state = await getAppConfigurationState(app.directory, basename(app.configuration.path)) + if (state.state !== 'connected-app') { + throw new AbortError('Error loading the app, please check your app configuration.') + } + const loadedConfiguration = await loadAppConfigurationFromState(state, app.specifications, app.remoteFlags ?? []) + + const loader = new AppLoader({ + loadedConfiguration, + previousApp: app, + }) + + return loader.loaded() +} + export async function loadAppUsingConfigurationState( configState: TConfig, { @@ -294,7 +312,11 @@ export async function loadDotEnv(appDirectory: string, configurationPath: string return dotEnvFile } -let alreadyShownCLIWarning = false +let didCheckForGlobalCLIWarning = false + +export function resetDidCheckForGlobalCLIWarning() { + didCheckForGlobalCLIWarning = false +} class AppLoader { private readonly mode: AppLoaderMode @@ -302,12 +324,14 @@ class AppLoader + private readonly previousApp: AppLinkedInterface | undefined - constructor({mode, loadedConfiguration}: AppLoaderConstructorArgs) { + constructor({mode, loadedConfiguration, previousApp}: AppLoaderConstructorArgs) { this.mode = mode ?? 'strict' this.specifications = loadedConfiguration.specifications this.remoteFlags = loadedConfiguration.remoteFlags this.loadedConfiguration = loadedConfiguration + this.previousApp = previousApp } async loaded() { @@ -320,15 +344,18 @@ class AppLoader !extensionInstancesWithKeys.some(([_, keys]) => keys.includes(key))) .filter((key) => { @@ -946,9 +976,7 @@ async function loadAppConfigurationFromState< case 'connected-app': { let gitTracked = false try { - gitTracked = !( - await checkIfIgnoredInGitRepository(configState.appDirectory, [configState.configurationPath]) - )[0] + gitTracked = await checkIfGitTracked(configState.appDirectory, configState.configurationPath) // eslint-disable-next-line no-catch-all/no-catch-all } catch { // leave as false @@ -976,6 +1004,16 @@ async function loadAppConfigurationFromState< } } +async function checkIfGitTracked(appDirectory: string, configurationPath: string) { + const gitIgnorePath = joinPath(appDirectory, '.gitignore') + if (!fileExistsSync(gitIgnorePath)) return false + const gitIgnoreContent = await readFile(gitIgnorePath) + const ignored = ignore.default().add(gitIgnoreContent) + const relative = relativePath(appDirectory, configurationPath) + const isTracked = !ignored.ignores(relative) + return isTracked +} + async function getConfigurationPath(appDirectory: string, configName: string | undefined) { const configurationFileName = getAppConfigurationFileName(configName) const configurationPath = joinPath(appDirectory, configurationFileName) diff --git a/packages/app/src/cli/services/dev/app-events/app-event-watcher-handler.ts b/packages/app/src/cli/services/dev/app-events/app-event-watcher-handler.ts index 00770aa1521..a85be4a1384 100644 --- a/packages/app/src/cli/services/dev/app-events/app-event-watcher-handler.ts +++ b/packages/app/src/cli/services/dev/app-events/app-event-watcher-handler.ts @@ -3,11 +3,10 @@ import {AppEvent, EventType, ExtensionEvent} from './app-event-watcher.js' import {appDiff} from './app-diffing.js' import {AppLinkedInterface} from '../../../models/app/app.js' import {ExtensionInstance} from '../../../models/extensions/extension-instance.js' -import {loadApp} from '../../../models/app/loader.js' -import {outputDebug} from '@shopify/cli-kit/node/output' +import {reloadApp} from '../../../models/app/loader.js' import {AbortError} from '@shopify/cli-kit/node/error' import {endHRTimeInMs, startHRTime} from '@shopify/cli-kit/node/hrtime' -import {basename} from '@shopify/cli-kit/node/path' +import {outputDebug} from '@shopify/cli-kit/node/output' /** * Transforms an array of WatcherEvents from the file system into a processed AppEvent. @@ -114,7 +113,7 @@ function AppConfigDeletedHandler(_input: HandlerInput): AppEvent { * - When an extension toml is updated */ async function ReloadAppHandler({event, app}: HandlerInput): Promise { - const newApp = await reloadApp(app) + const newApp = await reload(app) const diff = appDiff(app, newApp, true) const createdEvents = diff.created.map((ext) => ({type: EventType.Created, extension: ext})) const deletedEvents = diff.deleted.map((ext) => ({type: EventType.Deleted, extension: ext})) @@ -127,17 +126,12 @@ async function ReloadAppHandler({event, app}: HandlerInput): Promise { * Reload the app and returns it * Prints the time to reload the app to stdout */ -export async function reloadApp(app: AppLinkedInterface): Promise { +async function reload(app: AppLinkedInterface): Promise { const start = startHRTime() try { - const newApp = await loadApp({ - specifications: app.specifications, - directory: app.directory, - userProvidedConfigName: basename(app.configuration.path), - remoteFlags: app.remoteFlags, - }) + const newApp = await reloadApp(app) outputDebug(`App reloaded [${endHRTimeInMs(start)}ms]`) - return newApp as AppLinkedInterface + return newApp // eslint-disable-next-line @typescript-eslint/no-explicit-any } catch (error: any) { throw new Error(`Error reloading app: ${error.message}`) diff --git a/packages/app/src/cli/services/dev/app-events/app-event-watcher.test.ts b/packages/app/src/cli/services/dev/app-events/app-event-watcher.test.ts index eb1fbeb4865..0232a6f1129 100644 --- a/packages/app/src/cli/services/dev/app-events/app-event-watcher.test.ts +++ b/packages/app/src/cli/services/dev/app-events/app-event-watcher.test.ts @@ -2,7 +2,6 @@ import {AppEvent, AppEventWatcher, EventType, ExtensionEvent} from './app-event- import {OutputContextOptions, WatcherEvent, startFileWatcher} from './file-watcher.js' import {ESBuildContextManager} from './app-watcher-esbuild.js' import { - testApp, testAppAccessConfigExtension, testAppConfigExtensions, testAppLinked, @@ -11,7 +10,7 @@ import { testUIExtension, } from '../../../models/app/app.test-data.js' import {ExtensionInstance} from '../../../models/extensions/extension-instance.js' -import {loadApp} from '../../../models/app/loader.js' +import {loadApp, reloadApp} from '../../../models/app/loader.js' import {describe, expect, test, vi} from 'vitest' import {AbortSignal} from '@shopify/cli-kit/node/abort' import {flushPromises} from '@shopify/cli-kit/node/promises' @@ -233,7 +232,9 @@ describe('app-event-watcher when receiving a file event', () => { async ({fileWatchEvent, initialExtensions, finalExtensions, extensionEvents, needsAppReload}) => { // Given await inTemporaryDirectory(async (tmpDir) => { - vi.mocked(loadApp).mockResolvedValue(testApp({allExtensions: finalExtensions})) + const mockedApp = testAppLinked({allExtensions: finalExtensions}) + vi.mocked(loadApp).mockResolvedValue(mockedApp) + vi.mocked(reloadApp).mockResolvedValue(mockedApp) vi.mocked(startFileWatcher).mockImplementation(async (app, options, onChange) => onChange([fileWatchEvent])) const buildOutputPath = joinPath(tmpDir, '.shopify', 'bundle') @@ -286,15 +287,9 @@ describe('app-event-watcher when receiving a file event', () => { }) if (needsAppReload) { - expect(loadApp).toHaveBeenCalledWith({ - specifications: expect.anything(), - directory: expect.anything(), - // The app is loaded with the same configuration file - userProvidedConfigName: 'shopify.app.custom.toml', - remoteFlags: expect.anything(), - }) + expect(reloadApp).toHaveBeenCalled() } else { - expect(loadApp).not.toHaveBeenCalled() + expect(reloadApp).not.toHaveBeenCalled() } }) }, diff --git a/packages/app/src/cli/services/dev/app-events/file-watcher.ts b/packages/app/src/cli/services/dev/app-events/file-watcher.ts index 44123c86b65..4daa680fed9 100644 --- a/packages/app/src/cli/services/dev/app-events/file-watcher.ts +++ b/packages/app/src/cli/services/dev/app-events/file-watcher.ts @@ -69,9 +69,9 @@ export async function startFileWatcher( /** * Debounced function to emit the accumulated events. - * This function will be called at most once every 500ms to avoid emitting too many events in a short period. + * This function will be called at most once every 200ms to avoid emitting too many events in a short period. */ - const debouncedEmit = debounce(emitEvents, 500) + const debouncedEmit = debounce(emitEvents, 200) /** * Emits the accumulated events and resets the current events list. diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts index b462235f363..f02a8e6462f 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts @@ -28,7 +28,7 @@ import {WebType} from '../../../models/app/app.js' import {ensureDeploymentIdsPresence} from '../../context/identifiers.js' import {DeveloperPlatformClient} from '../../../utilities/developer-platform-client.js' import {AppEventWatcher} from '../app-events/app-event-watcher.js' -import {reloadApp} from '../app-events/app-event-watcher-handler.js' +import * as loader from '../../../models/app/loader.js' import {describe, test, expect, beforeEach, vi} from 'vitest' import {ensureAuthenticatedAdmin, ensureAuthenticatedStorefront} from '@shopify/cli-kit/node/session' import {Config} from '@oclif/core' @@ -42,7 +42,6 @@ vi.mock('../fetch.js') vi.mock('@shopify/cli-kit/node/environment') vi.mock('@shopify/theme') vi.mock('@shopify/cli-kit/node/themes/api') -vi.mock('../app-events/app-event-watcher-handler.js') beforeEach(() => { // mocked for draft extensions @@ -68,7 +67,6 @@ beforeEach(() => { role: 'theme', processing: false, }) - vi.mocked(reloadApp).mockResolvedValue(testAppLinked()) }) const appContextResult = { @@ -130,7 +128,7 @@ describe('setup-dev-processes', () => { allExtensions: [previewable, draftable, theme], }, }) - vi.mocked(reloadApp).mockResolvedValue(localApp) + vi.spyOn(loader, 'reloadApp').mockResolvedValue(localApp) const remoteApp: DevConfig['remoteApp'] = { apiKey: 'api-key', @@ -313,6 +311,7 @@ describe('setup-dev-processes', () => { }, } const localApp = testAppWithConfig() + vi.spyOn(loader, 'reloadApp').mockResolvedValue(localApp) const remoteApp: DevConfig['remoteApp'] = { apiKey: 'api-key', @@ -406,7 +405,7 @@ describe('setup-dev-processes', () => { allExtensions: [previewable, draftable, theme, functionExtension], }, }) - vi.mocked(reloadApp).mockResolvedValue(localApp) + vi.spyOn(loader, 'reloadApp').mockResolvedValue(localApp) const remoteApp: DevConfig['remoteApp'] = { apiKey: 'api-key', @@ -502,6 +501,8 @@ describe('setup-dev-processes', () => { }, }) + vi.spyOn(loader, 'reloadApp').mockResolvedValue(localApp) + const remoteApp: DevConfig['remoteApp'] = { apiKey: 'api-key', apiSecretKeys: [{secret: 'api-secret'}], @@ -594,7 +595,7 @@ describe('setup-dev-processes', () => { ], }, }) - vi.mocked(reloadApp).mockResolvedValue(localApp) + vi.spyOn(loader, 'reloadApp').mockResolvedValue(localApp) const remoteApp: DevConfig['remoteApp'] = { apiKey: 'api-key', diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts index 27c528eef38..86ee932b5a0 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts @@ -17,8 +17,8 @@ import {getProxyingWebServer} from '../../../utilities/app/http-reverse-proxy.js import {buildAppURLForWeb} from '../../../utilities/app/app-url.js' import {PartnersURLs} from '../urls.js' import {DeveloperPlatformClient} from '../../../utilities/developer-platform-client.js' -import {reloadApp} from '../app-events/app-event-watcher-handler.js' import {AppEventWatcher} from '../app-events/app-event-watcher.js' +import {reloadApp} from '../../../models/app/loader.js' import {getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' import {isTruthy} from '@shopify/cli-kit/node/context/utilities' import {getEnvironmentVariables} from '@shopify/cli-kit/node/environment' diff --git a/packages/cli-kit/src/public/node/json-schema.test.ts b/packages/cli-kit/src/public/node/json-schema.test.ts index fe83c8424d6..6b67c081ad4 100644 --- a/packages/cli-kit/src/public/node/json-schema.test.ts +++ b/packages/cli-kit/src/public/node/json-schema.test.ts @@ -52,7 +52,7 @@ describe('jsonSchemaValidate', () => { zod.object({foo: zod.number().max(99)}), {foo: 100}, ], - ])('matches the zod behaviour for %s', (_name, contract, zodVersion, subject) => { + ])('matches the zod behaviour for %s', (name, contract, zodVersion, subject) => { const zodParsed = zodVersion.safeParse(subject) expect(zodParsed.success).toBe(false) if (zodParsed.success) { @@ -61,7 +61,7 @@ describe('jsonSchemaValidate', () => { const zodErrors = zodParsed.error.errors.map((error) => ({path: error.path, message: error.message})) - const schemaParsed = jsonSchemaValidate(subject, contract) + const schemaParsed = jsonSchemaValidate(subject, contract, `test-${name}`) expect(schemaParsed.state).toBe('error') expect(schemaParsed.errors, `Converting ${JSON.stringify(schemaParsed.rawErrors)}`).toEqual(zodErrors) }) @@ -77,7 +77,7 @@ describe('jsonSchemaValidate', () => { }, 'x-taplo': {foo: 'bar'}, } - const schemaParsed = jsonSchemaValidate(subject, contract) + const schemaParsed = jsonSchemaValidate(subject, contract, 'test2') expect(schemaParsed.state).toBe('ok') }) @@ -121,7 +121,7 @@ describe('jsonSchemaValidate', () => { }, }, } - const schemaParsed = jsonSchemaValidate(subject, contract) + const schemaParsed = jsonSchemaValidate(subject, contract, 'test3') expect(schemaParsed.state).toBe('error') expect(schemaParsed.errors).toEqual([ { @@ -175,7 +175,7 @@ describe('jsonSchemaValidate', () => { }, }, } - const schemaParsed = jsonSchemaValidate(subject, contract) + const schemaParsed = jsonSchemaValidate(subject, contract, 'test4') expect(schemaParsed.state).toBe('error') expect(schemaParsed.errors).toEqual([ { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index a564cf327ca..019671fb1bc 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -200,6 +200,9 @@ importers: http-proxy: specifier: 1.18.1 version: 1.18.1 + ignore: + specifier: 6.0.2 + version: 6.0.2 proper-lockfile: specifier: 4.1.2 version: 4.1.2 @@ -11287,6 +11290,11 @@ packages: resolution: {integrity: sha512-5Fytz/IraMjqpwfd34ke28PTVMjZjJG2MPn5t7OE4eUCUNf8BAa7b5WUS9/Qvr6mwOQS7Mk6vdsMno5he+T8Xw==} engines: {node: '>= 4'} + /ignore@6.0.2: + resolution: {integrity: sha512-InwqeHHN2XpumIkMvpl/DCJVrAHgCsG5+cn1XlnLWGwtZBm8QJfSusItfrwx81CTp5agNZqpKU2J/ccC5nGT4A==} + engines: {node: '>= 4'} + dev: false + /immutable@3.7.6: resolution: {integrity: sha512-AizQPcaofEtO11RZhPPHBOJRdo/20MKQF9mBLnVkBoyHi1/zXK8fzVdnEpSV9gxqtnh6Qomfp3F0xT5qP/vThw==} engines: {node: '>=0.8.0'} From f339b3cdeaa28299f69ce53bdec9094f61226d9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 27 Nov 2024 13:46:46 +0100 Subject: [PATCH 3/4] Add more tests for gitTracked --- .../app/src/cli/models/app/loader.test.ts | 37 +++++++++++++++++++ packages/app/src/cli/models/app/loader.ts | 2 +- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/packages/app/src/cli/models/app/loader.test.ts b/packages/app/src/cli/models/app/loader.test.ts index 452a54ddcfb..36303c9af7e 100644 --- a/packages/app/src/cli/models/app/loader.test.ts +++ b/packages/app/src/cli/models/app/loader.test.ts @@ -2391,6 +2391,43 @@ wrong = "property" app_web_frontend_count: 0, }) }) + + test.skipIf(runningOnWindows)(`git_tracked metadata is false when ignored by the gitignore`, async () => { + const {webDirectory} = await writeConfig(linkedAppConfiguration, { + workspaces: ['packages/*'], + name: 'my_app', + dependencies: {}, + devDependencies: {}, + }) + await writeFile(joinPath(webDirectory, 'package.json'), JSON.stringify({})) + await writeFile(joinPath(tmpDir, '.gitignore'), 'shopify.app.toml') + + await loadTestingApp() + + expect(metadata.getAllPublicMetadata()).toEqual( + expect.objectContaining({ + cmd_app_linked_config_git_tracked: false, + }), + ) + }) + + test.skipIf(runningOnWindows)(`git_tracked metadata is true when there is no gitignore`, async () => { + const {webDirectory} = await writeConfig(linkedAppConfiguration, { + workspaces: ['packages/*'], + name: 'my_app', + dependencies: {}, + devDependencies: {}, + }) + await writeFile(joinPath(webDirectory, 'package.json'), JSON.stringify({})) + + await loadTestingApp() + + expect(metadata.getAllPublicMetadata()).toEqual( + expect.objectContaining({ + cmd_app_linked_config_git_tracked: true, + }), + ) + }) }) describe('getAppConfigurationFileName', () => { diff --git a/packages/app/src/cli/models/app/loader.ts b/packages/app/src/cli/models/app/loader.ts index 5ed0a197856..8b84526d59b 100644 --- a/packages/app/src/cli/models/app/loader.ts +++ b/packages/app/src/cli/models/app/loader.ts @@ -1006,7 +1006,7 @@ async function loadAppConfigurationFromState< async function checkIfGitTracked(appDirectory: string, configurationPath: string) { const gitIgnorePath = joinPath(appDirectory, '.gitignore') - if (!fileExistsSync(gitIgnorePath)) return false + if (!fileExistsSync(gitIgnorePath)) return true const gitIgnoreContent = await readFile(gitIgnorePath) const ignored = ignore.default().add(gitIgnoreContent) const relative = relativePath(appDirectory, configurationPath) From 992bf9422eb45e9ff5d5eb18060e15ea7e4b03d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 27 Nov 2024 14:56:04 +0100 Subject: [PATCH 4/4] Remove global didCheckForGlobalCLIWarning and update debounce to use leading --- .../app/src/cli/models/app/loader.test.ts | 2 - packages/app/src/cli/models/app/loader.ts | 47 ++----------------- .../app/validation/multi-cli-warning.ts | 31 ++++++++++++ .../services/app/config/link-service.test.ts | 2 +- .../services/dev/app-events/file-watcher.ts | 4 +- .../cli/services/generate/extension.test.ts | 1 + 6 files changed, 40 insertions(+), 47 deletions(-) create mode 100644 packages/app/src/cli/models/app/validation/multi-cli-warning.ts diff --git a/packages/app/src/cli/models/app/loader.test.ts b/packages/app/src/cli/models/app/loader.test.ts index 36303c9af7e..b8546629b2a 100644 --- a/packages/app/src/cli/models/app/loader.test.ts +++ b/packages/app/src/cli/models/app/loader.test.ts @@ -9,7 +9,6 @@ import { AppLoaderMode, getAppConfigurationState, loadConfigForAppCreation, - resetDidCheckForGlobalCLIWarning, } from './loader.js' import {LegacyAppSchema, WebConfigurationSchema} from './app.js' import {DEFAULT_CONFIG, buildVersionedAppSchema, getWebhookConfig} from './app.test-data.js' @@ -329,7 +328,6 @@ wrong = "property" test('shows warning if using global CLI but app has local dependency', async () => { // Given - resetDidCheckForGlobalCLIWarning() vi.mocked(globalCLIVersion).mockResolvedValue('3.68.0') vi.mocked(localCLIVersion).mockResolvedValue('3.65.0') const mockOutput = mockAndCaptureOutput() diff --git a/packages/app/src/cli/models/app/loader.ts b/packages/app/src/cli/models/app/loader.ts index 8b84526d59b..52170020453 100644 --- a/packages/app/src/cli/models/app/loader.ts +++ b/packages/app/src/cli/models/app/loader.ts @@ -18,6 +18,7 @@ import { AppCreationDefaultOptions, AppLinkedInterface, } from './app.js' +import {showMultipleCLIWarningIfNeeded} from './validation/multi-cli-warning.js' import {configurationFileNames, dotEnvFileNames} from '../../constants.js' import metadata from '../../metadata.js' import {ExtensionInstance} from '../extensions/extension-instance.js' @@ -48,11 +49,7 @@ import {AbortError} from '@shopify/cli-kit/node/error' import {outputContent, outputDebug, OutputMessage, outputToken} from '@shopify/cli-kit/node/output' import {joinWithAnd, slugify} from '@shopify/cli-kit/common/string' import {getArrayRejectingUndefined} from '@shopify/cli-kit/common/array' -import {renderInfo} from '@shopify/cli-kit/node/ui' -import {currentProcessIsGlobal} from '@shopify/cli-kit/node/is-global' import {showNotificationsIfNeeded} from '@shopify/cli-kit/node/notifications-system' -import {globalCLIVersion, localCLIVersion} from '@shopify/cli-kit/node/version' -import {jsonOutputEnabled} from '@shopify/cli-kit/node/environment' import ignore from 'ignore' const defaultExtensionDirectory = 'extensions/*' @@ -312,12 +309,6 @@ export async function loadDotEnv(appDirectory: string, configurationPath: string return dotEnvFile } -let didCheckForGlobalCLIWarning = false - -export function resetDidCheckForGlobalCLIWarning() { - didCheckForGlobalCLIWarning = false -} - class AppLoader { private readonly mode: AppLoaderMode private readonly errors: AppErrors = new AppErrors() @@ -351,7 +342,10 @@ class AppLoader { const websOfType = webs.filter((web) => web.configuration.roles.includes(webType)) diff --git a/packages/app/src/cli/models/app/validation/multi-cli-warning.ts b/packages/app/src/cli/models/app/validation/multi-cli-warning.ts new file mode 100644 index 00000000000..3516617b583 --- /dev/null +++ b/packages/app/src/cli/models/app/validation/multi-cli-warning.ts @@ -0,0 +1,31 @@ +import {renderInfo} from '@shopify/cli-kit/node/ui' +import {currentProcessIsGlobal} from '@shopify/cli-kit/node/is-global' +import {globalCLIVersion, localCLIVersion} from '@shopify/cli-kit/node/version' +import {jsonOutputEnabled} from '@shopify/cli-kit/node/environment' + +export async function showMultipleCLIWarningIfNeeded(directory: string, dependencies: {[key: string]: string}) { + // Show the warning if: + // - There is a global installation + // - The project has a local CLI dependency + // - The user didn't include the --json flag (to avoid showing the warning in scripts or CI/CD pipelines) + + const localVersion = dependencies['@shopify/cli'] && (await localCLIVersion(directory)) + const globalVersion = await globalCLIVersion() + + if (localVersion && globalVersion && !jsonOutputEnabled()) { + const currentInstallation = currentProcessIsGlobal() ? 'global installation' : 'local dependency' + + const warningContent = { + headline: `Two Shopify CLI installations found – using ${currentInstallation}`, + body: [ + `A global installation (v${globalVersion}) and a local dependency (v${localVersion}) were detected. +We recommend removing the @shopify/cli and @shopify/app dependencies from your package.json, unless you want to use different versions across multiple apps.`, + ], + link: { + label: 'See Shopify CLI documentation.', + url: 'https://shopify.dev/docs/apps/build/cli-for-apps#switch-to-a-global-executable-or-local-dependency', + }, + } + renderInfo(warningContent) + } +} diff --git a/packages/app/src/cli/services/app/config/link-service.test.ts b/packages/app/src/cli/services/app/config/link-service.test.ts index e1f400e47e5..417a72648c9 100644 --- a/packages/app/src/cli/services/app/config/link-service.test.ts +++ b/packages/app/src/cli/services/app/config/link-service.test.ts @@ -13,7 +13,7 @@ vi.mock('../../local-storage') vi.mock('@shopify/cli-kit/node/ui') vi.mock('../../dev/fetch.js') vi.mock('../../../utilities/developer-platform-client.js') - +vi.mock('../../../models/app/validation/multi-cli-warning.js') beforeEach(async () => {}) function buildDeveloperPlatformClient(): DeveloperPlatformClient { diff --git a/packages/app/src/cli/services/dev/app-events/file-watcher.ts b/packages/app/src/cli/services/dev/app-events/file-watcher.ts index 4daa680fed9..d3971c3cf4d 100644 --- a/packages/app/src/cli/services/dev/app-events/file-watcher.ts +++ b/packages/app/src/cli/services/dev/app-events/file-watcher.ts @@ -69,9 +69,9 @@ export async function startFileWatcher( /** * Debounced function to emit the accumulated events. - * This function will be called at most once every 200ms to avoid emitting too many events in a short period. + * This function will be called at most once every 300ms to avoid emitting too many events in a short period. */ - const debouncedEmit = debounce(emitEvents, 200) + const debouncedEmit = debounce(emitEvents, 300, {leading: true, trailing: true}) /** * Emits the accumulated events and resets the current events list. diff --git a/packages/app/src/cli/services/generate/extension.test.ts b/packages/app/src/cli/services/generate/extension.test.ts index 29b8728d864..4c4cbfc16a3 100644 --- a/packages/app/src/cli/services/generate/extension.test.ts +++ b/packages/app/src/cli/services/generate/extension.test.ts @@ -29,6 +29,7 @@ import * as git from '@shopify/cli-kit/node/git' import {joinPath, dirname} from '@shopify/cli-kit/node/path' import {slugify} from '@shopify/cli-kit/common/string' +vi.mock('../../models/app/validation/multi-cli-warning.js') vi.mock('@shopify/cli-kit/node/node-package-manager', async () => { const actual: any = await vi.importActual('@shopify/cli-kit/node/node-package-manager') return {