Skip to content

Remove packageManager, nodeDependencies, usesWorkspaces from App#7035

Merged
ryancbahan merged 10 commits intomainfrom
03-17-remove-project-fields-from-app
Mar 24, 2026
Merged

Remove packageManager, nodeDependencies, usesWorkspaces from App#7035
ryancbahan merged 10 commits intomainfrom
03-17-remove-project-fields-from-app

Conversation

@ryancbahan
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

ryancbahan commented Mar 18, 2026

@github-actions
Copy link
Copy Markdown
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
Copy Markdown
Contributor

github-actions bot commented Mar 18, 2026

Coverage report

St.
Category Percentage Covered / Total
🟢 Statements 82.16% 14931/18172
🟡 Branches 74.51% 7376/9899
🟢 Functions 81.29% 3776/4645
🟢 Lines 82.55% 14118/17102

Test suite run success

3933 tests passing in 1514 suites.

Report generated by 🧪jest coverage report action from f8d9783

@ryancbahan ryancbahan changed the base branch from rcb/project-integration to graphite-base/7035 March 18, 2026 21:42
@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 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
Copy Markdown
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.

@ryancbahan ryancbahan changed the base branch from rcb/project-integration to graphite-base/7035 March 19, 2026 14:12
@ryancbahan ryancbahan marked this pull request as draft March 20, 2026 02:48
@ryancbahan ryancbahan marked this pull request as ready for review March 20, 2026 15:31
@ryancbahan ryancbahan force-pushed the 03-17-remove-project-fields-from-app branch from d20e281 to 697f511 Compare March 20, 2026 15:35
@ryancbahan ryancbahan changed the base branch from graphite-base/7035 to rcb/project-integration March 20, 2026 15:35
@ryancbahan ryancbahan force-pushed the rcb/project-integration branch from 1e6e6de to 5e90c33 Compare March 23, 2026 17:11
@ryancbahan ryancbahan force-pushed the 03-17-remove-project-fields-from-app branch from 697f511 to 03a7fc8 Compare March 23, 2026 17:11
@ryancbahan ryancbahan changed the base branch from rcb/project-integration to graphite-base/7035 March 23, 2026 18:57
ryancbahan and others added 10 commits March 23, 2026 13:00
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>
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 force-pushed the 03-17-remove-project-fields-from-app branch from 03a7fc8 to f8d9783 Compare March 23, 2026 19:00
@ryancbahan ryancbahan changed the base branch from graphite-base/7035 to rcb/project-integration March 23, 2026 19:01
@ryancbahan ryancbahan requested a review from dmerand March 23, 2026 20:44
Copy link
Copy Markdown
Contributor

@dmerand dmerand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! My robot buddies had a couple of doc/test suggestions, but they're non-blocking.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Improvement: The removal of packageManager, nodeDependencies, and usesWorkspaces from AppInterface represents a significant architectural shift from a 'god object' pattern to separated concerns. While the code changes are clean, there's no documentation explaining why certain fields live in Project vs App, which could help future contributors understand the boundary and maintain it correctly.

Suggestion: Add JSDoc comments to both AppInterface and Project explaining the separation of concerns: AppInterface owns app-level configuration and extensions, while Project owns filesystem/environment infrastructure (package manager, dependencies, workspace detection). This would make the architectural intent explicit and help maintainers preserve the boundary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Suggestion: The reloadApp() function re-scans the filesystem via getAppConfigurationContext() on each reload, creating a fresh Project instance rather than persisting it in memory. This is likely correct for detecting new config files and extensions during dev sessions.

However, it's worth verifying through integration tests that:

  1. Filesystem changes (new extensions, configs) are properly picked up on reload
  2. Project re-scanning doesn't break dev session state
  3. Performance remains acceptable for repeated Project.load() calls

Previous PRs mentioned performance improvements from reducing scans - worth confirming those hold true in the reload path.

Suggestion: Add integration tests covering:

test('reloadApp picks up new extension added to filesystem', async () => {
  const app1 = await loadApp(...)
  await writeExtensionToml(...) // Add new extension to disk
  const app2 = await reloadApp(app1)
  expect(app2.allExtensions.length).toBe(app1.allExtensions.length + 1)
})

test('Project.load() performance acceptable for dev reload frequency', async () => {
  // Measure Project.load() time for typical app size
  // Verify it's under threshold (e.g., 100ms) for good dev UX
})

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this exact test is covered in the previous pr 👍

Base automatically changed from rcb/project-integration to main March 24, 2026 00:43
@ryancbahan ryancbahan added this pull request to the merge queue Mar 24, 2026
Merged via the queue into main with commit f1bf879 Mar 24, 2026
29 of 53 checks passed
@ryancbahan ryancbahan deleted the 03-17-remove-project-fields-from-app branch March 24, 2026 00:56
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.

2 participants