Skip to content

Remove packageManager, nodeDependencies, usesWorkspaces from App#7035

Open
ryancbahan wants to merge 10 commits intorcb/project-integrationfrom
03-17-remove-project-fields-from-app
Open

Remove packageManager, nodeDependencies, usesWorkspaces from App#7035
ryancbahan wants to merge 10 commits intorcb/project-integrationfrom
03-17-remove-project-fields-from-app

Conversation

@ryancbahan
Copy link
Contributor

@ryancbahan ryancbahan commented Mar 18, 2026

WHY are these changes introduced?

Depends on #7023. The App object currently acts as a god object, carrying environment concerns like packageManager, nodeDependencies, and usesWorkspaces alongside 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, usesWorkspaces fields and the updateDependencies() method from AppInterface and App class. Services now receive a Project instance explicitly:

  • AppInterface / App — removed the three fields and updateDependencies()
  • Options types — added project: Project to BuildOptions, DevOptions, DeployOptions, GenerateOptions, GenerateExtensionTemplateOptions
  • localAppContext — now returns {app, project} so commands can destructure and pass project through
  • installAppDependencies — takes Project instead of AppInterface
  • info — takes project param; backward compat in --json output preserved by injecting project fields into serialized output
  • loadOpaqueApp — surfaces packageManager for the link flow
  • Test infra — added testProject() helper, updated all affected tests

How to test your changes?

  1. pnpm type-check --projects=app passes
  2. npx vitest run packages/app/ passes
  3. Verify shopify app info --json still includes packageManager, nodeDependencies, usesWorkspaces in output

Measuring impact

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

Checklist

  • I've considered possible cross-platform impacts (Mac, Linux, Windows)
  • I've considered possible documentation changes

Copy link
Contributor Author

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.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions
Copy link
Contributor

Differences in type declarations

We 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:

  • Some seemingly private modules might be re-exported through public modules.
  • If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/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

@github-actions
Copy link
Contributor

github-actions bot commented Mar 18, 2026

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 82.21% 14881/18102
🟡 Branches 74.53% 7358/9872
🟢 Functions 81.37% 3765/4627
🟢 Lines 82.6% 14066/17030

Test suite run success

3896 tests passing in 1500 suites.

Report generated by 🧪jest coverage report action from d20e281

@ryancbahan ryancbahan changed the base branch from rcb/project-integration to graphite-base/7035 March 18, 2026 21:42
ryancbahan and others added 2 commits March 18, 2026 17:06
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>
@ryancbahan ryancbahan force-pushed the 03-17-remove-project-fields-from-app branch from dde9d0f to 550d006 Compare March 18, 2026 23:06
@ryancbahan ryancbahan changed the base branch from graphite-base/7035 to rcb/project-integration March 18, 2026 23:06
ryancbahan and others added 8 commits March 18, 2026 17:13
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>
@ryancbahan ryancbahan marked this pull request as ready for review March 19, 2026 01:09
@ryancbahan ryancbahan requested a review from a team as a code owner March 19, 2026 01:09
@github-actions
Copy link
Contributor

We detected some changes at packages/*/src and there are no updates in the .changeset.
If the changes are user-facing, run pnpm changeset add to track your changes and include them in the next release CHANGELOG.

Caution

DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant