Skip to content

Commit

Permalink
Improvements to App reload speed (#4924)
Browse files Browse the repository at this point in the history
### WHY are these changes introduced?

Improves app reloading performance by caching or avoiding expensive operations.

### WHAT is this pull request doing?

- `~200ms` Introduces a new `reloadApp` function that reuses data from the previous app instance.
- `~200ms` Caches JSON schema validators to avoid recompilation on each validation. 
- `~100ms` Replaces git ignore checking with a more efficient alternative: the `ignore` package
- `~1200ms` Improves handling of CLI version warning to only check once per command execution.
- Reduces the file-watcher debouncing time to 200ms

Before this PR, time to reload an app: `1500-2000ms`
New time: `~15-20ms` ❗ 
Improvement: `100x`

### How to test your changes?

1. Run `dev` on an app with multiple extensions using the app management API.
2. Make changes to extension configuration files.
3. Verify that reloading is faster and the app continues to function correctly.

### Measuring impact

- [x] n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix

### Checklist

- [x] I've considered possible cross-platform impacts (Mac, Linux, Windows)
- [x] I've considered possible documentation changes
  • Loading branch information
isaacroldan authored Nov 27, 2024
2 parents a25af5a + 992bf94 commit a1af910
Show file tree
Hide file tree
Showing 15 changed files with 166 additions and 87 deletions.
1 change: 1 addition & 0 deletions packages/app/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
40 changes: 39 additions & 1 deletion packages/app/src/cli/models/app/loader.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2353,6 +2353,7 @@ wrong = "property"
devDependencies: {},
})
await writeFile(joinPath(webDirectory, 'package.json'), JSON.stringify({}))
await writeFile(joinPath(tmpDir, '.gitignore'), '')

await loadTestingApp()

Expand All @@ -2364,7 +2365,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,
Expand All @@ -2388,6 +2389,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', () => {
Expand Down
91 changes: 46 additions & 45 deletions packages/app/src/cli/models/app/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ import {
BasicAppConfigurationWithoutModules,
SchemaForConfig,
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'
Expand Down Expand Up @@ -47,12 +49,8 @@ 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/*'

Expand Down Expand Up @@ -203,6 +201,8 @@ export class AppErrors {
interface AppLoaderConstructorArgs<TConfig extends AppConfiguration, TModuleSpec extends ExtensionSpecification> {
mode?: AppLoaderMode
loadedConfiguration: ConfigurationLoaderResult<TConfig, TModuleSpec>
// Used when reloading an app, to avoid some expensive steps during loading.
previousApp?: AppLinkedInterface
}

export async function checkFolderIsValidApp(directory: string) {
Expand Down Expand Up @@ -252,6 +252,21 @@ export async function loadApp<TModuleSpec extends ExtensionSpecification = Exten
return loader.loaded()
}

export async function reloadApp(app: AppLinkedInterface): Promise<AppLinkedInterface> {
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<TConfig extends AppConfigurationState>(
configState: TConfig,
{
Expand Down Expand Up @@ -294,20 +309,20 @@ export async function loadDotEnv(appDirectory: string, configurationPath: string
return dotEnvFile
}

let alreadyShownCLIWarning = false

class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionSpecification> {
private readonly mode: AppLoaderMode
private readonly errors: AppErrors = new AppErrors()
private readonly specifications: TModuleSpec[]
private readonly remoteFlags: Flag[]
private readonly loadedConfiguration: ConfigurationLoaderResult<TConfig, TModuleSpec>
private readonly previousApp: AppLinkedInterface | undefined

constructor({mode, loadedConfiguration}: AppLoaderConstructorArgs<TConfig, TModuleSpec>) {
constructor({mode, loadedConfiguration, previousApp}: AppLoaderConstructorArgs<TConfig, TModuleSpec>) {
this.mode = mode ?? 'strict'
this.specifications = loadedConfiguration.specifications
this.remoteFlags = loadedConfiguration.remoteFlags
this.loadedConfiguration = loadedConfiguration
this.previousApp = previousApp
}

async loaded() {
Expand All @@ -320,15 +335,21 @@ class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionS
const extensions = await this.loadExtensions(directory, configuration)

const packageJSONPath = joinPath(directory, 'package.json')
const name = await loadAppName(directory)
const nodeDependencies = await getDependencies(packageJSONPath)
const packageManager = await getPackageManager(directory)
await this.showMultipleCLIWarningIfNeeded(directory, nodeDependencies)

// These don't need to be processed again if the app is being reloaded
const name = this.previousApp?.name ?? (await loadAppName(directory))
const nodeDependencies = this.previousApp?.nodeDependencies ?? (await getDependencies(packageJSONPath))
const packageManager = this.previousApp?.packageManager ?? (await getPackageManager(directory))
const usesWorkspaces = this.previousApp?.usesWorkspaces ?? (await appUsesWorkspaces(directory))

if (!this.previousApp) {
await showMultipleCLIWarningIfNeeded(directory, nodeDependencies)
}

const {webs, usedCustomLayout: usedCustomLayoutForWeb} = await this.loadWebs(
directory,
configuration.web_directories,
)
const usesWorkspaces = await appUsesWorkspaces(directory)

const appClass = new App({
name,
Expand Down Expand Up @@ -392,35 +413,6 @@ class AppLoader<TConfig extends AppConfiguration, TModuleSpec extends ExtensionS
return parseConfigurationFile(schema, filepath, this.abortOrReport.bind(this), decode)
}

private async 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)
// - The warning hasn't been shown yet during the current command execution

const localVersion = dependencies['@shopify/cli'] && (await localCLIVersion(directory))
const globalVersion = await globalCLIVersion()

if (localVersion && globalVersion && !jsonOutputEnabled() && !alreadyShownCLIWarning) {
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)
alreadyShownCLIWarning = true
}
}

private validateWebs(webs: Web[]): void {
;[WebType.Backend, WebType.Frontend].forEach((webType) => {
const websOfType = webs.filter((web) => web.configuration.roles.includes(webType))
Expand Down Expand Up @@ -648,6 +640,7 @@ We recommend removing the @shopify/cli and @shopify/app dependencies from your p
)

// get all the keys from appConfiguration that aren't used by any of the results

const unusedKeys = Object.keys(appConfiguration)
.filter((key) => !extensionInstancesWithKeys.some(([_, keys]) => keys.includes(key)))
.filter((key) => {
Expand Down Expand Up @@ -946,9 +939,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
Expand Down Expand Up @@ -976,6 +967,16 @@ async function loadAppConfigurationFromState<
}
}

async function checkIfGitTracked(appDirectory: string, configurationPath: string) {
const gitIgnorePath = joinPath(appDirectory, '.gitignore')
if (!fileExistsSync(gitIgnorePath)) return true
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)
Expand Down
31 changes: 31 additions & 0 deletions packages/app/src/cli/models/app/validation/multi-cli-warning.ts
Original file line number Diff line number Diff line change
@@ -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)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -114,7 +113,7 @@ function AppConfigDeletedHandler(_input: HandlerInput): AppEvent {
* - When an extension toml is updated
*/
async function ReloadAppHandler({event, app}: HandlerInput): Promise<AppEvent> {
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}))
Expand All @@ -127,17 +126,12 @@ async function ReloadAppHandler({event, app}: HandlerInput): Promise<AppEvent> {
* Reload the app and returns it
* Prints the time to reload the app to stdout
*/
export async function reloadApp(app: AppLinkedInterface): Promise<AppLinkedInterface> {
async function reload(app: AppLinkedInterface): Promise<AppLinkedInterface> {
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}`)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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'
Expand Down Expand Up @@ -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')
Expand Down Expand Up @@ -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()
}
})
},
Expand Down
4 changes: 2 additions & 2 deletions packages/app/src/cli/services/dev/app-events/file-watcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 300ms to avoid emitting too many events in a short period.
*/
const debouncedEmit = debounce(emitEvents, 500)
const debouncedEmit = debounce(emitEvents, 300, {leading: true, trailing: true})

/**
* Emits the accumulated events and resets the current events list.
Expand Down
Loading

0 comments on commit a1af910

Please sign in to comment.