feat(services): data-only services boundary + import-free owned API surface - W-22419571#7569
Draft
peternhale wants to merge 66 commits into
Draft
feat(services): data-only services boundary + import-free owned API surface - W-22419571#7569peternhale wants to merge 66 commits into
peternhale wants to merge 66 commits into
Conversation
…onData types - W-22419571
Implements Task B.2 — data-only ProjectInfo mapper and getProjectInfo method. Changes: - Add projectInfoMapper.ts: toProjectInfo(SfProject, paths) → ProjectInfo Maps live SfProject to owned ProjectInfo DTO (boundary code) - Add getProjectInfo() to ProjectService: acquires SfProject, computes conventional SOQL/faux/typings paths, calls toProjectInfo - Wire getProjectInfo into ServicesContract and PlainServicesApi - Add projectInfoMapper.test.ts: TDD test covering field mapping The mapper is extension-internal boundary code (MAY import @salesforce/core), but produces services-owned ProjectInfo with no external imports. Path derivation: mirrors existing getSoqlMetadataPath/getFauxStandardObjectsPath/ getTypingsPath patterns (Global.SFDX_STATE_FOLDER + tools/sobjects segments). Verified: mapper test passes, compile clean, lint clean.
…mes - W-22419571 - Add deployMapper to map SDR DeployResult/RetrieveResult to owned DeployOutcome/RetrieveOutcome - Add buildComponentSet to ComponentSetService to dispatch SourceSpec to existing getters - Add deployFromSource(spec, opts) to MetadataDeployService - Add retrieveToSource(spec, opts) to MetadataRetrieveService - Wire new methods into contract and plainApi - Mapper test verifies owned outcome shape
…9571 Consumer trap fix: - deployFromSource() now takes NO options (both ignoreConflicts and checkOnly were silently ignored) - retrieveToSource() keeps only ignoreConflicts option (removed outputDir which was silently ignored) Removed: - DeployOptions type (deleted entirely - both fields were unwired) - RetrieveOptions.outputDir (only retrieveComponentSetToDirectory honors custom output path) Why: Exposing options that the implementation silently ignores creates a consumer trap. Better to expose nothing than expose broken options. These can be wired later when needed.
…anges - W-22419571
FIX 1: Prevent garbage OrgChange records from nameless ChangeResults.
- Add hasNameAndType type guard to filter changes before mapping
- Apply filter in getConflictChanges, getLocalChanges, getRemoteChanges
- Update toOrgChange signature to require name+type (removes ?? '' fallbacks)
- Add test verifying narrowed type requirement
FIX 2: Add applyIgnore option to getLocalChanges for symmetry with getRemoteChanges.
- Update contract, service implementation to accept opts?: { applyIgnore?: boolean }
- Filters ignored === true changes when applyIgnore is set
…nal - W-22419571 Make rawOutput non-optional to match @salesforce/templates CreateOutput contract where it is always provided. Also make created array readonly for consistency with other owned DTOs.
…rg - W-22419571 Migrated 2 of 4 apex-testing files from deprecated ConnectionService.getConnection() to the owned withDefaultOrg loan facade: - orgApexClassProvider.ts: tooling query for ApexClass body - testDiscovery.ts: Tooling API test discovery with pagination Both files now use api.withDefaultOrg(org => ...) via the plain services API. The callback receives a services-owned ServicesOrg with query/request operations. Type workaround: PromisifiedContract mapping loses the generic parameter from withDefaultOrg<R>, causing it to return Promise<unknown>. Used Effect.map with type assertion (eslint-disabled) to restore type safety until the contract type is improved. NOT migrated in this chunk: - apexTestRunUtils.ts: BLOCKED - requires raw Connection for TestService from @salesforce/apex-node (external package). This is a capability gap; TestService needs the full Connection object which ServicesOrg doesn't expose. - packageResolution.ts: Not called by any files in this chunk; will be migrated when its callers (testController.ts) are migrated in a future chunk. Verified: compile clean, lint clean (0 errors), no getConnection() usage in migrated files.
…DefaultOrg - W-22419571" This reverts commit 4dc4ef7.
…rved) - W-22419571
…W-22419571 Tasks 4-7 cluster: owned types re-export + deployManifest data-only deploy - Re-export DeployOutcome, FileResponseInfo, ComponentFailureInfo, SourceSpec, DeployFromSourceOptions from services/src/index.ts for metadata consumption - Create deployFromOutcome.ts as self-contained outcome presenter: inlines formatting, diagnostics, storage for owned DeployOutcome (keeps old helpers intact for unmigrated deployComponentSet) - Rewrite deployManifest command: conflict detection stays on existing ComponentSet path; deploy switches to data-only deployFromSource(spec) - Conflict retry restructured: performDeploy Effect.gen closes over spec/resolved so retry can re-invoke with same manifest URI - Channel output "Starting..." prints in command before deployFromSource - Live DeployResult no longer crosses boundary for this flow
Collapse duplicated deploy formatting to one owned-type implementation. Fix duplicate diagnostics collection. Add missing tests.
…nges opts - W-22419571
…-only - W-22419571
Switch deploySourcePath.ts and projectDeployStart.ts to deploy data-only via
MetadataDeployService.deployFromSource(spec, {ignoreConflicts:true}) +
deployFromOutcome(outcome), exactly mirroring the already-migrated
deployManifest.ts.
- deploySourcePath: spec = {kind:'paths', uris: resolvedUris.map(u => u.toString())}
- projectDeployStart: spec = {kind:'projectDirectories'}
Conflict detection STAYS on the existing ComponentSet path (deferred).
ConflictsDetectedError retry now re-issues the spec via the performDeploy
closure (closing over spec, NOT err.componentSet). The live DeployResult no
longer crosses the boundary for these two commands.
branches - W-22419571 BUG: deployFromOutcome.test.ts only called formatDeployOutput / getMergedDeployFailures directly — never invoked deployFromOutcome, so its 3 real behaviors (append formatted text to channel; applyDeployDiagnostics on failures-with-paths; raise DeployCompletedWithErrorsError on failures) were UNTESTED. FIX: Add real Effect tests that invoke deployFromOutcome(outcome) against mocked ChannelService + deployDiagnostics. Assert success path (formatted text appended, no diagnostics, no error) and failure paths (applyDeployDiagnostics called with filePath; no applyDeployDiagnostics without filePath; both fail with DeployCompletedWithErrorsError). Keep existing helper-level assertions; add unit-level coverage.
W-22419571 MINOR fixes: - deployMapper.test: add assertion for errorMessage (mapper sets response.errorMessage but no test asserted it) - README.md: remove stale createFromTemplate() mention (removed from public API; only createFromTemplateOwned() remains)
…22419571 Align 11 consumption/architecture docs with the final owned API + ADR-0021: correct owned-type field lists (DeployOutcome/RetrieveOutcome/ProjectInfo/ComponentSetInfo), mark removed public getters (describe/getLocalChangesAsComponentSet/createFromTemplate) as removed, document the bare-entry value-import rule, and clarify hand-authored owned types are the final approach.
…9571 - deployFromOutcome.test: widen runWithMocks requirement channel (deployFromOutcome statically requires FsService/ChannelService, both neutralized by jest.mock + the mocked getServicesApi). - org-browser index.test: add withDefaultOrg + getConnectionData to the ConnectionService mock (Phase-B members the service interface now requires).
…e - W-22419571 Phase E.2. Hand-authored import-free OwnedServicesApi (the Promise-returning data-only subset) published via a new `salesforcedx-vscode-services-types/owned` subpath — only owned DTOs + ServicesOrg + helpers + ICONS, zero @salesforce/jsforce/effect imports. Guards: - test-consumer fixture type-checks the owned surface with types:[] + skipLibCheck:false (no SDK/effect resolvable); wired as services-types check:consumer + test, added to the root test fan-out so it runs in the full-repo gate. - ownedServicesApiConformance.ts (src, enforced by real tsc since ts-jest isolatedModules cannot reliably evaluate type-level assertions) proves PlainServicesApi is a structural superset of OwnedServicesApi on the guarded members, so the type cannot silently drift. Two members are intentionally excluded from strict conformance (documented divergences): withDefaultOrg (PromisifiedContract generic-loss; owned keeps Promise<R>) and createFromTemplateOwned (owned templateType is string, deliberately wider to stay import-free). Also remove the orphaned `unable_to_retrieve_org_info` i18n key (both locales) left unused after the apex-replay-debugger getConnection migration; it blocked the whole-workspace lint.
…e - W-22419571 Completes Phase E.2 wiring (the OwnedServicesApi type + fixture landed in df410ec): - generateEntry.ts emits src/owned.ts (the import-free re-export bundle). - services-types package.json: ./owned subpath export, check:consumer + test wireit targets. - root package.json: add salesforcedx-vscode-services-types:test to the test fan-out.
…ervices-data-only # Conflicts: # packages/salesforcedx-vscode-services/src/terminal/terminalService.ts
mshanemc
reviewed
Jun 25, 2026
| * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
| */ | ||
| // Hand-authored, services-owned. NO imports from @salesforce/*, jsforce, or effect. | ||
| export type OrgChange = { |
Contributor
There was a problem hiding this comment.
I'd rather have these expressed as Effect schema transformations than as TS types.
so that shapes and transforms are expressed together.
[and then export the TS type derived from that]
mshanemc
reviewed
Jun 25, 2026
| export type ComponentInfo = { | ||
| readonly fullName: string; | ||
| readonly type: string; | ||
| readonly state?: string; |
Contributor
There was a problem hiding this comment.
"string" ? could probably be more specific than that
mshanemc
reviewed
Jun 25, 2026
| * | ||
| * @param obj - DescribeMetadataObject from jsforce metadata.describe() | ||
| */ | ||
| export const toMetadataTypeInfo = (obj: DescribeMetadataObject): MetadataTypeInfo => ({ |
Contributor
There was a problem hiding this comment.
yeah, definitely leaning Effect schema after seeing these.
Contributor
Author
There was a problem hiding this comment.
Effect schema forces consumer to use Effect
Contributor
Author
There was a problem hiding this comment.
These need to be plain.
Contributor
There was a problem hiding this comment.
no, like export the type derived from the effect schema.
mshanemc
reviewed
Jun 25, 2026
| /** sfdc-project.json `sfdcLoginUrl`, when set (used by auth flows). */ | ||
| readonly sfdcLoginUrl?: string; | ||
| /** sfdc-project.json `defaultLwcLanguage`, when set (e.g. 'typescript' | 'javascript'). */ | ||
| readonly defaultLwcLanguage?: string; |
Contributor
There was a problem hiding this comment.
why not make the types stricter when the values are known? is there a benefit of "string" ?
mshanemc
reviewed
Jun 25, 2026
|
|
||
| const mapSfLogLevel = (level: string): LogLevel.LogLevel => { | ||
| switch (level.toLowerCase()) { | ||
| // Defensive: config/env sources can yield a non-string at runtime despite the typed contract. |
mshanemc
reviewed
Jun 25, 2026
| query: async <T = Record<string, unknown>>(soql: string, opts?: QueryOpts): Promise<OwnedQueryResult<T>> => { | ||
| const api = opts?.tooling ? conn.tooling : conn; | ||
| const r = await api.query<JSForceRecord>(soql, { autoFetch: opts?.autoFetch, maxFetch: opts?.maxFetch }); | ||
| // jsforce's `Record[]` is structurally unrelated to the owned generic `T[]` plus differs in mutability, so a double-cast is required at this boundary. |
Contributor
There was a problem hiding this comment.
challenge to make it less hacky about types.
mshanemc
reviewed
Jun 25, 2026
| import type { Connection } from '@salesforce/core'; | ||
|
|
||
| /** Wraps a live Connection in the services-owned ServicesOrg facade. The Connection never escapes. */ | ||
| export const makeServicesOrg = (conn: Connection): ServicesOrg => ({ |
Contributor
There was a problem hiding this comment.
that's an odd name. this is the queryCrudService?
mshanemc
reviewed
Jun 25, 2026
| const workspaceInfo = yield* workspaceService.getWorkspaceInfoOrThrow(); | ||
|
|
||
| // Compute the conventional paths | ||
| const soqlMetadataPath = (yield* getSoqlMetadataPath()).fsPath; |
Contributor
There was a problem hiding this comment.
these should be vscode-uri, not string.
otherwise you shove all "safe on web" to the consumer. fsPath and string is bad news
mshanemc
reviewed
Jun 25, 2026
| "@salesforce/effect-ext-utils": "*", | ||
| "effect": "^3.21.2", | ||
| "@salesforce/vscode-i18n": "*", | ||
| "salesforcedx-vscode-services": "*", |
Contributor
There was a problem hiding this comment.
if you do that, all the dependencies of services (sfdx-core, jsforce, stl, sdr) are now in this extension, too.
absolutely not.
mshanemc
reviewed
Jun 25, 2026
| ); | ||
| // OwnedQueryResult is structurally compatible with QueryResult (both have done, totalSize, records) | ||
| const executeQuery = async (): Promise<QueryResult<JsonMap>> => { | ||
| const result = await api.withDefaultOrg(org => |
Contributor
There was a problem hiding this comment.
not a good pattern if the result comes back as unknown and then you have to assert that it is something.
mshanemc
reviewed
Jun 25, 2026
mshanemc
left a comment
mshanemc
left a comment
Contributor
There was a problem hiding this comment.
I'd like to exhaust other options before doing something like this.
a few of these might be desirable
- crudQuery service (to reduce some of the getConnection use cases)
- projectInfo (to avoid handing back the full project)
moving all the "foo as bar" into here instead of letting external consumers do it ( as Connection from their jsforce version) doesn't feel any better.
alternative to consider
- tsconfig tweaks to get rid of the relative paths
- typeof to derive the types from ex: Connection and re-export
- going into some of the libraries and exporting new duck-typeable non-class things (ex: I've long wanted to convert SDR's Deploy|Retrieve result to just be an object) for simpler types reimport/export or a ProjectInfo object from sfdx-core that just [zod | EffectSchema]-parses the file into an object
mshanemc
reviewed
Jun 25, 2026
mshanemc
left a comment
mshanemc
left a comment
Contributor
There was a problem hiding this comment.
another example would be a ComponentSet that's not a class but just an object you can read.
Some of the new "owned" objects would be better in the actual libraries.
…ervices-data-only # Conflicts: # package-lock.json # packages/salesforcedx-vscode-apex-testing/package.json # packages/salesforcedx-vscode-metadata/src/shared/retrieve/retrieveOutcome.ts # packages/salesforcedx-vscode-services/src/terminal/terminalService.ts # packages/salesforcedx-vscode-soql/src/commands/dataQuery.ts
… types Consumer extensions were incorrectly depending on the full salesforcedx-vscode-services extension package. They should only depend on the @salesforce/vscode-services package which provides the API surface types without pulling in the full extension implementation. Also added @salesforce/core dependency to apex-testing package which directly imports from it. Changes: - Changed devDependencies from salesforcedx-vscode-services to @salesforce/vscode-services in: - salesforcedx-vscode-soql - salesforcedx-vscode-apex-oas - salesforcedx-vscode-visualforce - salesforcedx-vscode-lightning - salesforcedx-vscode-apex-replay-debugger - salesforcedx-vscode-apex - salesforcedx-vscode-apex-log - salesforcedx-vscode-org - salesforcedx-vscode-lwc - Changed dependencies from salesforcedx-vscode-services to @salesforce/vscode-services in: - salesforcedx-vscode-metadata - salesforcedx-vscode-org-browser - Added @salesforce/core@^8.31.0 to salesforcedx-vscode-apex-testing dependencies
…meout - Updated retrieveOutcome.test.ts to work with Effect-based retrieveHasErrors that now requires RetrieveResult instead of RetrieveOutcome - Fixed terminalService.test.ts timeout expectation from 120s to 30s to match the new default timeout value from the merge - Added proper Effect layer mocking with ComponentSetService
Eliminate unsafe type assertions in servicesOrgAdapter.ts: - Add generic constraint `T extends Record<string, unknown>` to query/singleRecordQuery to prevent unconstrained generics and pass type parameters through to jsforce - Remove double-cast `as unknown as T` - now direct assignment with proper generic flow - Replace `as never` cast with type guard `isHttpMethod` and runtime validation - Add `Mutable<T>` helper for readonly→mutable conversions at serialization boundaries - Simplify arrow functions to single-line expressions (arrow-body-style) - Document remaining type casts as necessary boundary conversions with eslint-disable Related work item: W-22419571
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 does this PR do?
Reshapes the
salesforcedx-vscode-servicespublic API into a data-only boundary: it no longer loans live third-party-SDK instances (@salesforce/coreConnection/SfProject,source-deploy-retrieveComponentSet/DeployResult/RetrieveResult, jsforceDescribeMetadataObject,@salesforce/templatesCreateOutput) across the extension boundary. Instead it returns hand-authored owned data DTOs and lends a services-ownedServicesOrgfacade (withDefaultOrg(org => …)), so consumers get strong types without installing the Salesforce SDK oreffectand without version coupling.Why: the old shape hit the type-sharing trilemma — a consumer could only type a loaned
Connectionby (a) owning the type (impossible, it's foreign), (b) re-declaring it (never nominally assignable — TS comparesprivatemembers by declaring class), or (c) importing the real SDK (the dep burden we're avoiding). No 4th option. Data-only dissolves it: the surface no longer references foreign types, so an import-free published entry falls out for free.Highlights:
src/owned/*.ts(import nothing; guarded byownedTypestest). 3pp→owned mapping lives inside the service impls (*Mapper.ts).withDefaultOrglends an ownedServicesOrg(query/dml/request/identity,{tooling?}flag) — never a rawConnection. Build→deploy/retrieve moved behind spec-based methods (deployFromSource(spec),retrieveToSource(spec),retrieveRemoteChanges,retrieveMembers,describeProjectComponents(spec)); theComponentSetnever leaves services.new TestService(connection)from@salesforce/apex-node):getConnectionData()returns the data to rebuild aConnectionin the consumer's own dep space.getUserId).salesforcedx-vscode-services-types/ownedsubpath (OwnedServicesApi + DTOs + helpers + ICONS), proven by a consumer fixture that type-checks with no SDK/effect installed, plus a real-tsc conformance guard so the hand-authored type can't drift.Deliberately retained (tracked, gated on their consumers migrating; see ADR-0021): core
WorkspaceContext.getConnection()+getAuthFields()(published 2PP backward-compat); conflict detection +diffHelpers(need SDR path-resolution owned data can't express);deployOnSaveService+ delete-marked-ComponentSetdeploy.Docs:
effect, thewith*/functionality-shift hard parts).What issues does this PR fix or reference?
@W-22419571@
Functionality Before
Consumers received live 3pp SDK instances (e.g.
getConnection(): Connection,deploy(componentSet): DeployResult) and therefore had to install + version-match the Salesforce SDK (and transitivelyeffect) to type them.Functionality After
Consumers receive owned data DTOs / a
ServicesOrgloan facade / spec-based action methods returning owned outcomes. No SDK oreffectinstall needed for the data-only surface; the newsalesforcedx-vscode-services-types/ownedentry type-checks with neither installed (enforced by a consumer-gate test). No user-visible behavior change — channel output, diagnostics, conflict handling, and result storage are preserved.