Remove packageManager, nodeDependencies, usesWorkspaces from App#7035
Remove packageManager, nodeDependencies, usesWorkspaces from App#7035ryancbahan wants to merge 10 commits intorcb/project-integrationfrom
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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 declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/themes/api.d.ts@@ -5,7 +5,6 @@ export type ThemeParams = Partial<Pick<Theme, 'name' | 'role' | 'processing' | '
export type AssetParams = Pick<ThemeAsset, 'key'> & Partial<Pick<ThemeAsset, 'value' | 'attachment'>>;
export declare function fetchTheme(id: number, session: AdminSession): Promise<Theme | undefined>;
export declare function fetchThemes(session: AdminSession): Promise<Theme[]>;
-export declare function findDevelopmentThemeByName(name: string, session: AdminSession): Promise<Theme | undefined>;
export declare function themeCreate(params: ThemeParams, session: AdminSession): Promise<Theme | undefined>;
export declare function fetchThemeAssets(id: number, filenames: Key[], session: AdminSession): Promise<ThemeAsset[]>;
export declare function deleteThemeAssets(id: number, filenames: Key[], session: AdminSession): Promise<Result[]>;
packages/cli-kit/dist/public/node/themes/theme-manager.d.ts@@ -8,8 +8,8 @@ export declare abstract class ThemeManager {
protected abstract removeTheme(): void;
protected abstract context: string;
constructor(adminSession: AdminSession);
- findOrCreate(name?: string, role?: Role): Promise<Theme>;
- fetch(name?: string, role?: Role): Promise<Theme | undefined>;
+ findOrCreate(): Promise<Theme>;
+ fetch(): Promise<Theme | undefined>;
generateThemeName(context: string): string;
create(themeRole?: Role, themeName?: string): Promise<Theme>;
}
\ No newline at end of file
|
Coverage report
Test suite run success3896 tests passing in 1500 suites. Report generated by 🧪jest coverage report action from d20e281 |
Services now receive explicit Project dependencies instead of reading
env fields from the App god object. This makes dependencies visible
and decouples services from App's shape.
- Remove packageManager, nodeDependencies, usesWorkspaces fields and
updateDependencies() method from AppInterface and App class
- Update installAppDependencies to take Project instead of AppInterface
- Add project to BuildOptions, DevOptions, DeployOptions, GenerateOptions,
GenerateExtensionTemplateOptions, and info() params
- Update localAppContext to return {app, project}
- Update all commands to destructure and pass project through
- Preserve backward compat in `shopify app info --json` by injecting
project fields into the serialized output
- Add testProject() helper for test infrastructure
- Update loadOpaqueApp to surface packageManager for link flow
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Unexport TestProjectOptions and LocalAppContextOutput (only used internally) - Remove unused imports: yarnLockfile, pnpmLockfile, pnpmWorkspaceFile, captureOutput, AppConfiguration - Fix import order for project.js in app.test-data.ts - Remove orphaned <<<<<<< HEAD conflict marker in loader.test.ts - Remove extra blank line in context.test.ts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
87728e2 to
5f12f8e
Compare
dde9d0f to
550d006
Compare
Replace toBeDefined() with concrete expected values (npm, {}, false)
since setupRealApp creates a minimal project with deterministic
metadata. Rename test to reflect it verifies filesystem loading.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The earlier rebase used git checkout --theirs which took a version of
app.ts that dropped configPath and AppConfiguration & {path: string}
changes that should have been preserved.
Restored app.ts from rcb/project-integration, then applied only the
intended removals: packageManager, nodeDependencies, usesWorkspaces,
and updateDependencies(). Also updated loader.ts to use
this.loadedConfiguration.configPath instead of appConfiguration.path
in webhook/config extension creation methods.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused schemaType param from testApp/testAppLinked - Remove unused _extensionDirectories param from createExtensionInstances - Fix unsafe client_id cast to type-safe extraction with '' default - Add backward-compat assertions for project fields in info JSON test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Include path in parseConfigurationFile fallback return to match
declared return type (was returning {} without path)
- Use || instead of ?? for effectiveClientId so empty string falls
through to configClientId (preserves original falsy-check behavior)
- Fix info JSON test to expect 'yarn' (testProject default), not 'npm'
- Add TODO noting path injection is transitional (Phase 1 extraction)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nFile
The rebase left parseConfigurationFile injecting {path: filepath} into
parsed configs, but the parent branch already removed this. This caused
path to leak into TOML output (snapshot failures) and configuration
objects returned from link() (deepEqual failures).
- Remove & {path: string} return type from parseConfigurationFile
- Remove delete rawConfig.path (no longer needed)
- Remove 'path' from configKeysThatAreNeverModules
- Remove 'path' from blockedKeys in rewriteConfiguration
- Remove path from DEFAULT_CONFIG test fixture
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests were expecting path inside configuration object, but path is no longer injected by parseConfigurationFile. Use app.configPath (the separate field on App) instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Instances The rebase replaced isAppConfigSpecification(spec) with uidStrategy === 'single', which is NOT equivalent — uidStrategy 'single' includes webhook subscriptions (experience: 'extension'), while isAppConfigSpecification only matches experience: 'configuration'. Also restored the missing WebhookSubscriptionSpecIdentifier exclusion filter that the rebase dropped. Without this fix, config extensions aren't properly identified during deploy, causing the API to reject with "at least one specification file is required". Verified: all 12 E2E tests pass locally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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. |

WHY are these changes introduced?
Depends on #7023. The App object currently acts as a god object, carrying environment concerns like
packageManager,nodeDependencies, andusesWorkspacesalongside configuration data. With the Project model now owning these fields, services should receive them as explicit dependencies rather than reaching into App.WHAT is this pull request doing?
Removes
packageManager,nodeDependencies,usesWorkspacesfields and theupdateDependencies()method fromAppInterfaceandAppclass. Services now receive aProjectinstance explicitly:AppInterface/App— removed the three fields andupdateDependencies()project: ProjecttoBuildOptions,DevOptions,DeployOptions,GenerateOptions,GenerateExtensionTemplateOptionslocalAppContext— now returns{app, project}so commands can destructure and pass project throughinstallAppDependencies— takesProjectinstead ofAppInterfaceinfo— takesprojectparam; backward compat in--jsonoutput preserved by injecting project fields into serialized outputloadOpaqueApp— surfacespackageManagerfor the link flowtestProject()helper, updated all affected testsHow to test your changes?
pnpm type-check --projects=apppassesnpx vitest run packages/app/passesshopify app info --jsonstill includespackageManager,nodeDependencies,usesWorkspacesin outputMeasuring impact
Checklist