Uh oh!
There was an error while loading. Please reload this page.
Relocate paired-module interfaces into per-directory types.d.ts (phase 1) - #182
Merged
Conversation
…e 1) Interfaces that lived in `.d.ts` files paired with a same-named `.js` (which TypeScript treats as authoritative, shadowing the JS) are moved into per-directory `types.d.ts` files that do not coincide with any `.js`: - dep_graph + manifest interfaces -> new src/core/types.d.ts - CapabilityRegistryHandle -> registry/types.d.ts - KernelRuntime, activation/loader/paths arg+result types -> runtime/types.d.ts - DispatchOptions, ConfiguredMenuOption, integration types -> cli/types.d.ts The paired `.d.ts` files keep only their function/class declarations for now (phase 2 deletes them so the `.js` JSDoc becomes the single source of truth). All consumers import the relocated names from `types.d.ts`. Dropped the unused kernel-type re-export blocks in manifest.d.ts and capabilities.d.ts. Renamed integration's `CommandResult` to `IntegrationCommandResult` to avoid colliding with the existing cli `CommandResult`. Re-enabled `skipLibCheck: false`. That surfaced latent issues now fixed: daemon/types.d.ts imported ExtendedSource/SinkRegistry and BootKernelResult from the wrong modules; daemon/status.js dropped an optional-chain on a now-honestly-typed source registry; and SinkInstanceConfig's optional fields were not assignable to the JsonObject index, so it now carries its own `JsonValue | undefined` index instead of extending JsonObject. npm test (1473 pass), npm run typecheck, npm run lint all green.
Review follow-up: instead of dropping `extends JsonObject` and giving
SinkInstanceConfig its own `JsonValue | undefined` index (which silenced
the TS2411 but lost structural assignability to JsonObject), model it as
`JsonObject & { schedule?; encoder? }`. This fixes the original optional-
property-vs-index error and keeps a sink config usable anywhere a
JsonObject is expected.…port - integration.d.ts: `./integration` is a public package entry, so relocating its option/result interfaces into ./types.d.ts dropped the named type exports (external consumers could no longer `import` them). Re-export them from the public entry to hold the API stable; `CommandResult` keeps its original public name (aliased from the internal `IntegrationCommandResult`). This is the one place a re-export is warranted. - daemon/types.d.ts: drop the unused `BootKernelResult` import carried along by the skipLibCheck fix.
platypii
commented
Jun 28, 2026
ContributorAuthor
Cloud review (ultrareview) summaryThe cloud review surfaced two findings, both now addressed (commit 9c5427b): 1. Public integration types dropped (normal). 2. Dead No correctness, runtime, or convention issues were found. |
Uh oh!
There was an error while loading. Please reload this page.
platypii added a commit
that referenced
this pull request
Jun 28, 2026
…2) (#183) Phase 1 (#182) relocated interfaces into per-directory types.d.ts, leaving eight paired .d.ts files holding only function/class declarations that duplicated their sibling .js. A foo.d.ts next to foo.js shadows the JS, so those declarations, not the JSDoc, were the authoritative module types. Delete the eight now-redundant declaration files so each .js JSDoc becomes the single source of truth: dep_graph, manifest, runtime/{activation,loader,paths}, cli/{dispatch,core_commands}, registry/capabilities Verified by emitting declarations from the JS via tsc and diffing against the deleted .d.ts: every signature is structurally identical (named aliases like ResolveDependenciesOptions / CreatePluginPathsArgs / ManifestLoadResult just inline to their definitions). One real drift fixed: capabilities.js carried a loose @returns that dropped fromProvider from the public type; it now returns the named CapabilityRegistryHandle, matching the old declaration exactly. cli/integration.d.ts is intentionally kept: it is a public package entry (package.json exports "./integration") whose option/result types must stay importable by name, and a .js cannot re-export types at runtime. typecheck, lint (423 files), and npm test (1473 pass) all green.
philcunliffe added a commit
that referenced
this pull request
Jul 30, 2026
Type-only trim. Each removed declaration had exactly one occurrence in the tree (its own definition): no importer, no `@import`, no test reference, no `@ref` annotation, and no re-export from any public entry. - `src/core/runtime/types.d.ts`: `CreateKernelRuntimeArgs`, `CreateActivationContextArgs`, `CreatePluginPathsArgs`, `ActivatePluginsArgs`, `ActivatePluginsResult`. Leftovers from the paired-.d.ts consolidation (#182/#183); `activation.js` and `paths.js` spell these param shapes inline in JSDoc instead. - `src/core/types.d.ts`: `ResolveDependenciesOptions`, `ManifestLoadResult`. `manifest.js` writes `Promise<LoadedManifest|FailedManifest>` directly; both members stay. - `src/core/cli/types.d.ts`: `WalkthroughResult`, orphaned by "delete the dead walkthrough" (#241). Removing those left `PluginPaths` and `SourceWithholdResolver` as unused type imports in `src/core/runtime/types.d.ts`; dropped both. No runtime code changed. npm test: 3005 pass / 8 fail / 1 skipped, the same eight `leave-command.test.js` failures present on origin/master. npm run typecheck clean. `npm run smoke -- core_boot_noop` ok. Co-authored-by: test <test@test.com> Co-authored-by: Claude <noreply@anthropic.com>
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Why
.d.tsfiles paired with a same-named.jsare treated by TypeScript as the authoritative type for that module, shadowing the.js. That forced us to hand-write and keep function signatures in sync. This is phase 1 of fixing that: relocate the interfaces out of those paired files into per-directorytypes.d.tsfiles (names that don't coincide with any.js). Phase 2 will delete the now function-only paired.d.tsso each.js's JSDoc becomes the single source of truth.What moved
src/core/types.d.tsCapabilityRegistryHandle->registry/types.d.tsKernelRuntime, activation/loader/paths arg+result types ->runtime/types.d.tsDispatchOptions,ConfiguredMenuOption, integration types ->cli/types.d.tsEach paired
.d.tskeeps only its function/class declarations; consumers now import the relocated names fromtypes.d.ts. Dropped the unused kernel-type re-export blocks inmanifest.d.tsandcapabilities.d.ts(no re-export shims). Renamed integration'sCommandResulttoIntegrationCommandResultto avoid colliding with the existing cliCommandResult.Lib check re-enabled
Set
skipLibCheck: false. That surfaced latent issues, now fixed:daemon/types.d.tsimportedExtendedSource/SinkRegistryandBootKernelResultfrom the wrong modules (was silentlyany).daemon/status.jsdropped an optional-chain on a now-honestly-typed source registry.SinkInstanceConfig extends JsonObjectwith optional fields was ill-formed understrictNullChecks(TS2411 x2, pre-existing). Remodeled asJsonObject & { schedule?; encoder? }— fixes the error and keeps it assignable toJsonObject. Flagging since it touchescollectivus-plugin-kernel-types.d.ts.Checks
npm test— 1473 pass, 0 failnpm run typecheck— green (with lib check on)npm run lint— green/code-review high— no correctness or convention findings; one suggestion applied (the intersection-type model above)