Halve CLI startup time by not loading the app for non-app commands - #8199
Draft
isaacroldan wants to merge 1 commit into
Draft
Halve CLI startup time by not loading the app for non-app commands#8199isaacroldan wants to merge 1 commit into
isaacroldan wants to merge 1 commit into
Conversation
`shopify version` goes from 369ms to 180ms (-51%), measured on the production entry point with a clean bundle and an isolated config dir. The dominant cost was the `public_command_metadata` hook. oclif fans it out on every command via analytics, and `@shopify/app`'s implementation statically imported `localAppContext`, dragging in the whole app graph -- including theme-check-common, whose ohm-js Liquid grammar is compiled from source at module evaluation time. That alone accounted for ~190ms of the 199ms that ran after the command had already printed its output. Making the import dynamic is not sufficient, because the hook always calls `localAppContext()`. Instead it now gates on `getCurrentCommandId()`, which cli-kit's BaseCommand already sets to the canonical oclif id, so no hook signature or fanout plumbing had to change. Also included: - analytics: check the `analyticsDisabled`/`alwaysLog*` gates before building the payload, so nothing is built when neither destination can receive it. Guarded with `isVerbose()` so `--verbose` still logs the would-be payload. - base-command: import `session.js` on demand, only when `--auth-alias` is passed. `commandSessionId` moves into a new dependency-free `private/node/session/command-session-id.ts` so clearing the selection stays correct without loading the session/identity/API graph. (~10ms pre-output.) - otel: `unref()` the throttle timer. It does not fire today, but a second `onForceFlush` within the window would pin the process for up to 5s. - notifications: skip the background refresh -- which spawns a whole extra CLI process -- when the cached payload is under an hour old. - app-init: skip the fsync'd atomic write when the store is already empty. Note: non-app commands no longer report `app_*` analytics fields. They could not produce meaningful ones without an app in the working directory, but a non-app command run inside an app directory now loses them too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Differences in type declarationsWe 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:
New type declarationspackages/cli-kit/dist/private/node/session/command-session-id.d.ts/**
* Get the session id selected for the current command, if any.
*
* @returns The selected session id, or undefined when no alias was selected.
*/
export declare function getCommandSessionId(): string | undefined;
/**
* Select a stored session for the current command process.
*
* @param sessionId - The session id to select, or undefined to clear the selection.
*/
export declare function setCommandSessionId(sessionId: string | undefined): void;
Existing type declarationspackages/cli-kit/dist/private/node/session.d.ts@@ -90,7 +90,6 @@ export declare function setLastSeenUserIdAfterAuth(id: string): void;
*/
export declare function getLastSeenAuthMethod(): Promise<AuthMethod>;
export declare function setLastSeenAuthMethod(method: AuthMethod): void;
-export declare function setCommandSessionId(sessionId: string | undefined): void;
export interface EnsureAuthenticatedAdditionalOptions {
noPrompt?: boolean;
forceRefresh?: boolean;
packages/cli-kit/dist/public/node/base-command.d.ts@@ -22,6 +22,13 @@ declare abstract class BaseCommand extends Command {
}>;
protected environmentsFilename(): string | undefined;
protected failMissingNonTTYFlags(flags: FlagOutput, requiredFlags: string[]): void;
+ /**
+ * Resolving an alias needs the session/identity/API graph, so it is imported on demand. Almost no
+ * invocation passes `--auth-alias`, and clearing the selection only needs the tiny state module.
+ *
+ * @param alias - The account alias passed via `--auth-alias`, if any.
+ */
+ private selectSessionAlias;
private resultWithEnvironment;
/**
* Tries to load an environment to forward to the command. If no environment
packages/cli-kit/dist/public/node/local-storage.d.ts@@ -34,6 +34,14 @@ export declare class LocalStorage<T extends Record<string, any>> {
* @throws BugError if an unexpected error occurs.
*/
delete<TKey extends keyof T>(key: TKey): void;
+ /**
+ * The number of values held in the local storage.
+ *
+ * Useful to avoid an unnecessary write when there is nothing to clear.
+ *
+ * @returns The number of stored keys.
+ */
+ get size(): number;
/**
* Clear the local storage (delete all values).
*
|
graygilmore
reviewed
Jul 29, 2026
Comment on lines
+25
to
+26
| // Imported lazily so that non-app commands never pay for the app graph. | ||
| const {localAppContext} = await import('../services/app-context.js') |
Contributor
There was a problem hiding this comment.
If non-app commands don't call logAppContextMetadata at all is this part necessary?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
shopify versiongoes from 369 ms → 180 ms (−51%). The win applies to every command that isn't anappcommand.versiontotalversiontail after outputversiontime to first outputapp --helptime to first outputMeasured on
bin/run.js(the production entry point —bin/dev.jssetsSHOPIFY_CLI_ENV=development, which disables analytics and takes a different path), withHOMEpointed at a throwaway dir so the 300/day analytics rate limit couldn't skew repeats, 15 runs, load average under 4.Why
The dominant cost was the
public_command_metadatahook. oclif fans it out on every command viaanalytics.ts, and@shopify/app's implementation statically importedlocalAppContext— dragging in the whole app graph, including@shopify/theme-check-common, whose ohm-js Liquid grammar is compiled from source at module-evaluation time. That was ~190 ms of the 199 ms that ran after the command had already printed its output.Making the import dynamic is not sufficient: the hook always calls
localAppContext(), so the import happens anyway. It now gates ongetCurrentCommandId(), which cli-kit'sBaseCommandalready sets to the canonical oclif id — so no hook signature or fanout plumbing had to change.Also included
analyticsDisabled/alwaysLog*gates beforebuildPayload(), so nothing is built when neither destination can receive it. Guarded withisVerbose()so--verbosestill logs the would-be payload.session.json demand, only when--auth-aliasis passed (setCurrentSessionAlias(undefined)returns immediately).commandSessionIdmoves into a new dependency-freeprivate/node/session/command-session-id.tsso clearing the selection stays correct without loading the session/identity/API graph. This is the ~10 ms pre-output gain.unref()the throttle timer. It doesn't fire today (onForceFlushis called once per process), but a second call within the window would pin the process for up to 5 s.Trade-off to review
Non-app commands no longer report
app_*analytics fields. They couldn't produce meaningful ones without an app in the working directory, but a non-app command run inside an app directory now loses them too. Worth a look from whoever owns those dashboards.Testing
cli-kit,app,clitype-checkandlintpass across all 13 projects (CI-equivalent,--skip-nx-cache)knipclean;refresh-code-documentationproduces no churnversion,--version,app/theme/store/auth/config --help,app build --help,--verbose, and--auth-alias <bad>(still errors correctly through the lazy import)Two test files timed out on full-suite runs — different files on different runs, both pass in isolation, both slow timing-based suites (16 s and 26 s of test time against a 5 s per-test limit). Pre-existing flakes.
Unrelated bug found while measuring
packages/cli/project.jsondeclaresbuildwithoutputs: ["{workspaceRoot}/dist"](should be{projectRoot}/dist) andbundlewith noinputs/outputsat all. nx consequently serves stale bundles: with these changes fully stashed,diststill contained the compiled new code. Every number above comes fromrm -rf dist && nx bundle cli --skip-nx-cache. Not fixed here — it deserves its own PR since touching nx caching could affect CI.Dead end worth recording
Stubbing out 1.5 MB of
graphql+ OpenTelemetry + Bugsnag chunks saved −1.4 ms. Bytes loaded are nearly free — V8 lazy-parses and the CJS bodies never execute. Only module bodies that actually run cost anything, which is why one ohm-js grammar compile outweighed 2 MB of dependencies. Same for the 3.5 MB TypeScript compiler chunk (pulled in by@oclif/core's guardedrequire('typescript')): never executed, ~2 ms. The remaining ~213 ms of pre-output latency lives inbase-command's import graph, not in bundle size.🤖 Generated with Claude Code