Add Project domain model for filesystem abstraction#7022
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Coverage report
Test suite run success3849 tests passing in 1480 suites. Report generated by 🧪jest coverage report action from af2a018 |
ryancbahan
force-pushed
the
rcb/project-model
branch
2 times, most recently
from
c7fcd8b to
d60ce08
Compare
ryancbahan
changed the title
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
Introduces a new Project domain model that abstracts the filesystem for Shopify app projects. Project discovers all config files, extension files, web files, dotenv files, hidden config, and project metadata in a single scan. - Project: discovers and holds all filesystem state without interpreting it. Supports multi-config projects (shopify.app.*.toml). - ActiveConfig: represents the selected app configuration, derived from Project. Resolves config-specific dotenv and hidden config. - Config selection functions: resolveDotEnv, resolveHiddenConfig, extensionFilesForConfig, webFilesForConfig — pure functions that derive config-specific state from Project. 48 tests covering project discovery, config selection, file filtering, dotenv resolution, hidden config lookup, multi-config scenarios, and integration parity with the existing loader. All new code — zero changes to existing files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ryancbahan
force-pushed
the
rcb/project-model
branch
from
d60ce08 to
2edfd38
Compare
dmerand
left a comment
dmerand
left a comment
There was a problem hiding this comment.
I'm aligned with the overall approach of creating a shared abstraction for local config and reducing continued direct access. There are still some concerns about consistency with making this a drop-in that need to be addressed.
| */ | ||
| export async function selectActiveConfig(project: Project, userProvidedConfigName?: string): Promise<ActiveConfig> { | ||
| let configName = userProvidedConfigName | ||
| const source: ConfigSource = configName ? 'flag' : 'cached' |
There was a problem hiding this comment.
🐛 Bug: When no userProvidedConfigName is given and no cached config exists, the code falls through to use the default config. However, the source is set as 'cached' on line 56 even though it should indicate 'default'. This means the source will incorrectly report 'cached' for the default fallback case, which could confuse debugging and telemetry.
Suggestion: Consider determining the source after the actual config selection is finalized, or expand the ConfigSource type to include 'default':
export type ConfigSource = 'flag' | 'cached' | 'default'Then update the logic:
let source: ConfigSource
if (userProvidedConfigName) {
source = 'flag'
} else if (cachedConfigName) {
source = 'cached'
} else {
source = 'default'
}| */ | ||
| export function resolveDotEnv(project: Project, activeConfigPath: string): DotEnvFile | undefined { | ||
| const shorthand = getAppConfigurationShorthand(activeConfigPath) | ||
| if (shorthand) { | ||
| const specificName = `${dotEnvFileNames.production}.${shorthand}` | ||
| const specific = project.dotenvFiles.get(specificName) | ||
| if (specific) return specific | ||
| } |
There was a problem hiding this comment.
resolveDotEnv() changes the existing selection semantics for non-default configs. Historically loader.ts loaded the config-specific dotenv file only (shopify.app.staging.toml → .env.staging) and did not fall back to .env; there is prior history here from the earlier dotenv-selection bugfix work.
With this implementation we now try .env.<shorthand> and then silently fall back to .env. That is not behavior-neutral: a non-default config can now inherit base env values it previously would not see, which can mask a missing config-specific dotenv file or leak values across configs.
I think we should either preserve the old semantics here, or call this out as an intentional behavior change and add regression tests proving the fallback is desired.
| if (!Array.isArray(configDirs) || configDirs.length === 0) { | ||
| // Default: extensions/* — filter project files by path prefix | ||
| return project.extensionConfigFiles.filter((file) => { | ||
| const relPath = file.path.replace(project.directory, '').replace(/^\//, '') |
There was a problem hiding this comment.
🐛 Bug: The path manipulation uses string replacement with hardcoded forward slashes which will fail on Windows. The pattern .replace(project.directory, '').replace(/^\//, '') only removes leading forward slashes, not backslashes used by Windows. This same pattern appears on lines 78, 90, and 109. Your existing codebase in loader.ts uses the relativePath utility from @shopify/cli-kit/node/path for cross-platform compatibility.
Suggestion:
| const relPath = file.path.replace(project.directory, '').replace(/^\//, '') | |
| const relPath = relativePath(project.directory, file.path) |
You'll also need to add the import:
import {relativePath} from '@shopify/cli-kit/node/path'| // Filter to files that are within the active config's declared directories | ||
| const dirPrefixes = (configDirs as string[]).map((dir) => { | ||
| // Remove trailing glob (e.g., "custom/*" → "custom/") | ||
| return dir.replace(/\*.*$/, '') | ||
| }) | ||
|
|
||
| return project.extensionConfigFiles.filter((file) => { | ||
| const relPath = file.path.replace(project.directory, '').replace(/^\//, '') | ||
| return dirPrefixes.some((prefix) => relPath.startsWith(prefix)) | ||
| }) | ||
| } | ||
|
|
||
| /** | ||
| * Filter web config files to those belonging to the active config's | ||
| * web_directories. If not specified, returns all web files. | ||
| * @public | ||
| */ | ||
| export function webFilesForConfig(project: Project, activeConfig: TomlFile): TomlFile[] { | ||
| const configDirs = activeConfig.content.web_directories | ||
| if (!Array.isArray(configDirs) || configDirs.length === 0) { | ||
| return project.webConfigFiles | ||
| } | ||
|
|
||
| const dirPrefixes = (configDirs as string[]).map((dir) => dir.replace(/\*.*$/, '')) | ||
|
|
||
| return project.webConfigFiles.filter((file) => { | ||
| const relPath = file.path.replace(project.directory, '').replace(/^\//, '') | ||
| return dirPrefixes.some((prefix) => relPath.startsWith(prefix)) |
There was a problem hiding this comment.
The old loader used the configured extension_directories / web_directories globs directly. Here we reduce each configured pattern to a simple prefix by stripping from the first * and then doing startsWith(...). That is a semantic weakening: more complex globs can now over-match.
For example, a pattern shaped like foo/*/extensions effectively becomes foo/, so files outside the intended glob but under that prefix will still be treated as belonging to the config. That makes this refactor less obviously behavior-preserving than it first appears.
I think we should add tests for nontrivial glob shapes before treating this as a safe equivalence refactor. If only simple prefix-like patterns are meant to be supported, it would be better to codify that in the schema/docs rather than silently weakening the matching behavior here.
| }) | ||
|
|
||
| return project.extensionConfigFiles.filter((file) => { | ||
| const relPath = file.path.replace(project.directory, '').replace(/^\//, '') |
There was a problem hiding this comment.
🐛 Bug: Same hardcoded forward slash issue as line 78. This will cause extension filtering to fail on Windows when using custom extension_directories.
Suggestion:
| const relPath = file.path.replace(project.directory, '').replace(/^\//, '') | |
| const relPath = relativePath(project.directory, file.path) |
| const dirPrefixes = (configDirs as string[]).map((dir) => dir.replace(/\*.*$/, '')) | ||
|
|
||
| return project.webConfigFiles.filter((file) => { | ||
| const relPath = file.path.replace(project.directory, '').replace(/^\//, '') |
There was a problem hiding this comment.
🐛 Bug: Same hardcoded forward slash issue in web config filtering. Consider extracting this to a helper function since the pattern appears three times.
Suggestion:
| const relPath = file.path.replace(project.directory, '').replace(/^\//, '') | |
| const relPath = relativePath(project.directory, file.path) |
| * Does NOT select which config is active or resolve modules. | ||
| */ | ||
| static async load(startDirectory: string): Promise<Project> { | ||
| const directory = await findProjectRoot(startDirectory) | ||
|
|
||
| // Discover all app config files | ||
| const appConfigFiles = await discoverAppConfigFiles(directory) | ||
| if (appConfigFiles.length === 0) { | ||
| throw new AbortError(`Could not find a Shopify app TOML file in ${directory}`) | ||
| } | ||
|
|
||
| // Discover extension files from all app configs' extension_directories (union). | ||
| // Configs that don't specify extension_directories use the default (extensions/*). | ||
| const allExtensionDirs = new Set<string>() | ||
| for (const appConfig of appConfigFiles) { | ||
| const dirs = appConfig.content.extension_directories | ||
| if (Array.isArray(dirs)) { | ||
| for (const dir of dirs) allExtensionDirs.add(dir as string) | ||
| } else { | ||
| allExtensionDirs.add(DEFAULT_EXTENSION_DIR) | ||
| } | ||
| } | ||
| const extensionConfigFiles = await discoverExtensionFiles(directory, [...allExtensionDirs]) | ||
|
|
||
| // Discover web files from all app configs' web_directories (union) | ||
| const allWebDirs = new Set<string>() | ||
| for (const appConfig of appConfigFiles) { | ||
| const dirs = appConfig.content.web_directories | ||
| if (Array.isArray(dirs)) { | ||
| for (const dir of dirs) allWebDirs.add(dir as string) | ||
| } | ||
| } | ||
| const webConfigFiles = await discoverWebFiles(directory, allWebDirs.size > 0 ? [...allWebDirs] : undefined) | ||
|
|
||
| // Project metadata | ||
| const packageJSONPath = joinPath(directory, 'package.json') | ||
| const hasPackageJson = await fileExists(packageJSONPath) | ||
| const packageManager = hasPackageJson ? await getPackageManager(directory) : 'unknown' | ||
| const nodeDependencies = hasPackageJson ? await getDependencies(packageJSONPath) : {} | ||
| const usesWorkspaces = hasPackageJson ? await detectUsesWorkspaces(directory) : false | ||
|
|
||
| // Dotenv: discover ALL .env* files in the root | ||
| const dotenvFiles = await discoverDotEnvFiles(directory) | ||
|
|
||
| // Hidden config: store the raw .shopify/project.json content | ||
| const hiddenConfigRaw = await loadRawHiddenConfig(directory) | ||
|
|
||
| return new Project({ | ||
| directory, | ||
| packageManager, | ||
| nodeDependencies, | ||
| usesWorkspaces, |
There was a problem hiding this comment.
The PR description says Project.load() replaces the old repeated scans, but the current load path still appears to do another project-wide config discovery pass afterwards. loadAppConfigurationFromState() still calls getAllLinkedConfigClientIds(...), and that helper re-globs/parses config files instead of reusing the already-loaded Project data.
So this is not yet a strict "one scan replaces 6+ scans" path in practice: the new project-wide discovery happens here, and then we still pay for at least one extra config scan for metadata. If the performance reduction is a core claim, I think we should either thread project through loadAppConfigurationFromState() / getAllLinkedConfigClientIds(), or soften the claim until the full call path actually reuses the same discovery result end-to-end.
If you want to keep the claim, it would be good to benchmark the real before/after load path rather than asserting a single-scan architecture that the current implementation does not fully achieve yet.
There was a problem hiding this comment.
This is addressed in followup pr.
| const directory = await findProjectRoot(startDirectory) | ||
|
|
||
| // Discover all app config files | ||
| const appConfigFiles = await discoverAppConfigFiles(directory) | ||
| if (appConfigFiles.length === 0) { | ||
| throw new AbortError(`Could not find a Shopify app TOML file in ${directory}`) | ||
| } | ||
|
|
||
| // Discover extension files from all app configs' extension_directories (union). | ||
| // Configs that don't specify extension_directories use the default (extensions/*). | ||
| const allExtensionDirs = new Set<string>() | ||
| for (const appConfig of appConfigFiles) { | ||
| const dirs = appConfig.content.extension_directories | ||
| if (Array.isArray(dirs)) { | ||
| for (const dir of dirs) allExtensionDirs.add(dir as string) | ||
| } else { | ||
| allExtensionDirs.add(DEFAULT_EXTENSION_DIR) | ||
| } | ||
| } | ||
| const extensionConfigFiles = await discoverExtensionFiles(directory, [...allExtensionDirs]) | ||
|
|
||
| // Discover web files from all app configs' web_directories (union) | ||
| const allWebDirs = new Set<string>() | ||
| for (const appConfig of appConfigFiles) { | ||
| const dirs = appConfig.content.web_directories | ||
| if (Array.isArray(dirs)) { | ||
| for (const dir of dirs) allWebDirs.add(dir as string) |
There was a problem hiding this comment.
Project.load() now eagerly parses every shopify.app*.toml, plus every extension/web TOML reachable from the union of all configs' extension_directories / web_directories, before active-config selection happens. That is a behavior change from loader.ts, which historically treated the selected config as the unit of loading: it only parsed the chosen app config and only globbed module TOMLs from that config's directories, and even getAllLinkedConfigClientIds explicitly ignored parse errors in sibling configs.
The regression is that a malformed or half-migrated inactive config/file can now prevent the active config from loading. Concretely, an unrelated broken shopify.app.other.toml, inactive extension TOML, or inactive web TOML will now fail Project.load() before selectActiveConfig() gets a chance to filter.
Can we keep discovery shallow for non-active files, or defer TOML parsing until after active-config selection? At minimum, perhaps add regression coverage proving the active config still loads when an inactive app/extension/web TOML is malformed.
…rsing - ConfigSource: add 'default' variant so telemetry correctly distinguishes no-flag-no-cache fallback from a cached selection - DotEnv: remove fallback from config-specific to base .env for non-default configs — preserves old semantics where .env.staging missing meant no dotenv, not silent inheritance from .env - Windows paths: replace hardcoded forward-slash string manipulation with relativePath() from cli-kit in extensionFilesForConfig/webFilesForConfig - Glob-to-prefix: add comment documenting the simplification and its limits - Eager parsing: wrap TomlFile.read() in try/catch during discovery so a malformed inactive config or extension TOML is skipped with a debug log instead of blocking the active config from loading Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ryancbahan
force-pushed
the
rcb/project-model
branch
from
509aacf to
8964add
Compare
…allback The old code computed configSource before cache resolution, so when neither a flag nor a cached config was set, the default shopify.app.toml fallback was mislabeled as 'cached' instead of 'default'. Move the source determination after resolution to match the three-way logic already used by selectActiveConfig. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The regex filter in discoverDotEnvFiles used \w+ which excludes hyphens, so .env.my-staging would not be discovered for a shopify.app.my-staging.toml config. Change to [\w-]+ to match the config shorthand naming convention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…onfig
relativePath() returns OS-native separators (backslashes on Windows),
so startsWith('extensions/') comparisons would fail. Normalize to
forward slashes before prefix matching.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…atches Without a trailing slash, extension_directories like "ext-a" would also match "ext-a-other/..." paths. Normalize prefixes to always end with '/' before startsWith comparison. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dmerand
left a comment
dmerand
left a comment
There was a problem hiding this comment.
This LGTM as a basis for centralizing project config. Thanks for making changes!
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
2 participants
WHY are these changes introduced?
The CLI currently discovers filesystem state (config files, extensions, webs, dotenv, hidden config, dependencies) across many scattered code paths — the loader, app-context, link service, and individual commands all independently scan the filesystem. This makes it hard to reason about what files the system knows about, leads to redundant disk reads, and tightly couples Shopify domain logic to OS/filesystem concerns (the latter of which is the highest concern as we start to consider isomorphic CLI operations.)
Introducing a
Projectmodel separates these concerns. Project is a pure filesystem abstraction — it discovers what's on disk and provides access to it. It doesn't interpret configs as modules, select which config is active, or know about the Shopify platform. This separation is a foundation for making the CLI codebase more isomorphic by abstracting away OS/filesystem operations behind a clean interface.WHAT is this pull request doing?
Adds three new modules under
models/project/:Project— discovers and holds all filesystem state for a Shopify app project. Finds the project root by walking up from the working directory, discovers allshopify.app*.tomlfiles (supporting multi-config projects), extension TOMLs, web TOMLs, dotenv files, hidden config (.shopify/project.json), and package metadata. Single scan, no duplicate reads.ActiveConfig— represents the selected app configuration (one of potentially many in the project). Derived from Project viaselectActiveConfig(), which resolves flag > cache > default, handles stale cache, and derives config-specific dotenv and hidden config. A sibling to Project, never nested inside it.resolveDotEnv,resolveHiddenConfig,extensionFilesForConfig,webFilesForConfig. Pure functions that derive config-specific state from the Project's raw data.How to test your changes?
Measuring impact
Checklist