Uh oh!
There was an error while loading. Please reload this page.
fix(data-objectstack): createObjectStackAdapter declares the adapter it returns - #7503
Merged
Merged
Conversation
…it returns The factory returned `new ObjectStackAdapter(config)` while declaring `DataSource<T>`. A wider value is assignable to a narrower annotation, so nothing ever failed to compile — the loss was entirely on the reading side. Measured against the shipped `dist/index.d.ts` with the doc-snippet gate's own compiler options, nine reads through `ReturnType<typeof createObjectStackAdapter>` failed with TS2339: getClient, getCacheStats, invalidateCache, clearCache, getConnectionState, isConnected, onConnectionStateChange, onBatchProgress and setSystemCapabilities. Eight are exactly the members the package README's API Reference documents; the ninth is the one the factory's own JSDoc links to. Same probe after this change: 0 diagnostics. Option A of the card. B (widening `DataSource` itself) would make every other data source declare caching, connection state and batch progress it does not have; C (documenting a cast) teaches a cast around a declaration that is merely narrower than the value. `ObjectStackAdapter` was already exported from the package's only entry, so nothing is exported by implication, and no comment, ADR or test pinned the narrow return — the commit that added autoReconnect / maxReconnectAttempts / reconnectDelay to the factory's config bag left the members observing those features off its declared return in the same change. Callers are unaffected: `const ds: DataSource = createObjectStackAdapter(…)` still compiles and still gives the narrow surface. The README note saying the page could not yet teach the factory's shape is removed, and the four sections built on the adapter-only members now continue from Basic Setup's factory call. `adapterFactoryReturn.types.test.ts` pins the card's TS2339 reproduction inverted, with a control for option B and a control for swappability. Part of #7323 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMrWaQw3XS5DxTHxp4yRyC
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-project-manager
commented
Sep 3, 2026
CollaboratorAuthor
Clause-② in-seat contract review — PASS WITH REQUIRED AMENDMENTS. |
…idened factory return (objectui#7323)
Two text-only amendments from the in-seat contract review on the pull request.
No code changes: the return-type widening, the type-level pin and the README
rewiring are untouched.
Amendment 1 — content/docs/utilities/data-objectstack.mdx contradicted the
shipped types. The four sites the review named: the Quick Start prose saying the
factory "returns a `DataSource`"; the API Reference blurb "Factory returning a
`DataSource`"; the signature fragment ending `}): DataSource<T>;` together with
its marker's claim to have been checked against the shipped `dist/index.d.ts`;
and the `ObjectStackAdapter` section built on "hold the class type to reach
these", a distinction the factory no longer has. That section is rewritten rather
than deleted, with a "What changed" paragraph so a reader who followed the old
advice can see why constructing the class by hand is no longer needed.
Two further sites in the same file taught the same erased distinction and are
corrected with them: the Mutations paragraph ("hold the adapter at its class type
... before calling them") and Troubleshooting -> Authentication Errors
("Connection introspection lives on the adapter class ... so hold it at the class
type"); both examples now continue from the factory call, matching how the README
limb of this pull request was rewired. `onMutation` was also misfiled under
"Adapter-only" while being an optional member of `DataSource`; it moves in with
`bulk` and `batchTransaction`.
The fragment's marker no longer asserts an unqualified "checked against the
shipped d.ts". Because the block is declared, `check:doc-snippet-types` never
compiles it, so the marker now states that the agreement is hand-checked rather
than gate-enforced -- which is why the stale return type survived there. The gate
and the `declared` marker keyword are unchanged.
Amendment 2 — the changeset understated the published surface by more than 2x.
It presented the card's nine measured TS2339 reads as the delta; the factory's
declared return is now the class itself, so the delta is every public member of
`ObjectStackAdapter` -- 20 beyond `DataSource`, independently re-derived here as
`Exclude<keyof ObjectStackAdapter<unknown>, keyof DataSource<unknown>>` against
the built `dist/index.d.ts` (20 names, matching the review). The README-documents
sentence is corrected: eight of the nine measured reads are on that list, but the
list itself is nine adapter-only members -- `connect()` was documented all along
and simply was not measured. The "Not a breaking change for callers." heading is
reworded to lead with the one shape that does break, so the CHANGELOG line no
longer reads as "no break" above the caveat describing it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EMrWaQw3XS5DxTHxp4yRyC…ui#7323) Follow-up to the previous commit, on my own prose in the same amendment: - Drop the "Until v17.7" version claim. The package is at 17.6.0 and the changeset is a minor, but the released number is decided by the fixed group at release time, so the page should not assert it. - Remove a duplicated bullet list. `bulk` and `batchTransaction` were listed once under the `DataSource` members and again under the optional-members paragraph. `onMutation` now joins them in the single list, spelled `onMutation?` like its neighbours, with the correction noted in the paragraph instead. - Say "six bullets cover nine members" rather than "those nine", since the list groups related members onto shared lines, and make the parenthetical naming the other eleven members read as illustrative rather than exhaustive -- it named four seams out of eleven. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMrWaQw3XS5DxTHxp4yRyC
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-project-manager
commented
Sep 3, 2026
CollaboratorAuthor
✅ Both amendments verified — |
Uh oh!
There was an error while loading. Please reload this page.
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
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.
Fixes#7323
createObjectStackAdapterreturnednew ObjectStackAdapter(config)while declaring the sharedDataSourceinterface as its return. A wider value is assignable to a narrower annotation, so nothing ever failed to compile — the loss was entirely on the reading side: every adapter-only member was erased from the type the factory hands back, while staying on the object it hands back.Measured before the change, compiling against the shipped
packages/data-objectstack/dist/index.d.tswith the doc-snippet gate's own compiler options (strict, bundler resolution): 9 x TS2339, not the 4 the card lists —getClient,getCacheStats,invalidateCache,clearCache,getConnectionState,isConnected,onConnectionStateChange,onBatchProgress,setSystemCapabilities. Eight of those are exactly the members the README's API Reference — Methods list documents; the ninth is the one the factory's own JSDoc links to ([ADR-0066] See ObjectStackAdapter.setSystemCapabilities). The same probe after this change: 0 diagnostics.The two questions that decided the shape
The triage set these as a stop-condition and both are answered from the code, on
origin/maine17605309.Q1 — is
ObjectStackAdapterexported from the package's public entry? Yes, already.packages/data-objectstack/src/index.ts:2148readsexport class ObjectStackAdapter, andtsup.config.tshas exactly one entry,src/index.ts. The class is in the shippeddist/index.d.tsexport list (verified in the built artifact, not inferred). Two pin tests already assert the exported spelling in source —cloud-surface-retired-4152.pin.test.ts:114andv3-deep-integration-retired-4241.pin.test.ts:135— andapps/console/src/dataSource.ts:14re-exports it by name. So this PR exports nothing by implication: the export list indist/index.d.tsis unchanged, name for name, before and after.Q2 — is the narrow return deliberate? No, and there are two pieces of evidence against it.
a5d817061("Add connection state monitoring, auto-reconnect, and batch progress") addedautoReconnect/maxReconnectAttempts/reconnectDelayto the factory's own config bag while leaving the members that observe those features off the factory's declared return. The same change configured a feature it made unobservable through its own entry point. That is an oversight shape, not an encapsulation decision.41b7ce3ce, points the reader atObjectStackAdapter.setSystemCapabilities— a member its declared return hides.Nothing anywhere pins the return: no comment, no ADR, no test, no
Equalassertion, and no swappability note. Searched the export list,package.jsonfiles/exports, ADR-0066, the sibling adapters, and every test that names the factory.Had either answer come back the other way this would have been a docs-only PR. They did not, so it is option A — and swappability, the property the narrow return was suspected of protecting, is not lost: a wider return is assignable to the narrower annotation, so
const ds: DataSource = createObjectStackAdapter(...)still compiles and still gives the narrow surface. There is a control test that says exactly that.Why A and not B or C
B (add the missing members to
DataSource) would make every otherDataSourceimplementation declare caching, connection state and batch progress it does not have — those are this adapter's concerns. C (document a cast) teaches a cast around a declaration that is merely narrower than the value, which is the opposite ofdeclared = enforced. A is one line and makes declared match shipped for every documented member at once.The pin
packages/data-objectstack/src/adapterFactoryReturn.types.test.ts— the card's TS2339 reproduction, inverted. It reads all nine members through the factory's return, asserts the return's identity withEqual(not mere assignability — two adapter-shaped declarations are mutually assignable, so only identity can tell "returns THE adapter" from "returns something adapter-shaped"), and carries two controls:_NotOnDataSource— the adapter-only members stay ABSENT from the sharedDataSource. This is what makes the reads a statement about the factory's return rather than about every data source, and it fires on option B._StillADataSource— the widened return is still assignable toDataSource.Both controls are independent of the return annotation, so reverting the source change turns the reads red and leaves the controls green. That split is measured below, not asserted.
Which tree the pin exercises. Two, deliberately. The test is compiled by
pnpm --filter @object-ui/data-objectstack type-check, whose program includes the wholesrc/**(tests included) and resolves./indexfromsrc— proven with--listFiles: 1 hit for the new file, 55 test files,src/index.tspresent.@object-ui/typesin that same program resolves frompackages/types/dist, so the type used for the controls is the shipped one. The README blocks are thedisthalf:check:doc-snippetscompiles them against the builtdist/*.d.ts(its own resolution control prints'@object-ui/types' was successfully resolved to packages/types/dist/index.d.ts).Documentation
The README note naming this card (added by #5174 batch 8) is removed. The four sections built on the adapter-only members — Metadata Caching, Connection State Monitoring, Batch Operation Progress, Troubleshooting → Cache Issues — now continue from Basic Setup's
createObjectStackAdapter(...)call instead of hand-declaring the class, so the page teaches one shape and the doc-snippet gate pins the fix againstdist. The other README blocks still declare the class where they only needDataSource-level members; that is truthful (the class is public and documented) and rewriting them buys nothing, so they are left alone.setSystemCapabilitiesis still absent from the README's Methods list. It was absent before this PR too, it is a separate doc gap, and adding it here would be scope this card did not ask for.Verification — every number below was observed, at head
922ca54ecpnpm --filter @object-ui/data-objectstack type-checktsc --noEmitechoedpnpm exec vitest run --maxWorkers=2 packages/data-objectstack/(repo root — a package-dir run is refused by objectui#3378's guard)check:doc-snippetsnode scripts/check-doc-snippet-types.mjscheck:readme-exportspnpm check:readme-exportsnode scripts/check-changeset-presence.mjscheck:doc-fencesnode scripts/check-doc-fence-languages.mjsnode scripts/check-control-bytes.mjspnpm --filter @object-ui/data-objectstack lintno-explicit-any; the new file contributes 0 findings (eslint --format jsonon the two changed source files: new test 0/0)Every gate result above is quoted from the gate's own verdict line; exit codes were captured by redirecting first and reading
$?before any pipe.Dependency closure built first (
pnpm --filter '@object-ui/data-objectstack...' build, then the doc-snippet gate's own derived--build-filter: 26 packages, 34 turbo tasks, then--filter='./packages/*'forcheck:readme-exports, which had reported 2 unbuilt packages — a "could not run", not a defect).Lint scope, declared: the table's lint row is the changed package only, not the repo.
pnpm linthere isturbo run lint(each package's owneslint .), and CI runs it in full regardless. One reading that needs stating: a separate probe witheslint --no-inline-configreports 4 errors insrc/index.tsat lines 1369–1389. Those are the documentedeslint-disable-next-line no-consolesites for the specLoggerbinding (objectui#4029), they are ~4300 lines from either of my edits, and they are invisible to the lint CI actually runs because it honours inline config. Not introduced here, and not silently omitted either.Reverse verification — red/green split, both directions proven on disk
Method: commit first, then mutate
packages/data-objectstack/src/index.tsback to the narrow return undertrap ... EXIT INT TERMwith an absolute restore path, prove the mutation landed by counting the injected and removed text (not by the editor's exit code) and by comparing blob hashes, then restore and prove the restore by state.Mutation landed: injected-text count 0 → 1, removed-text count 1 → 0; mutated blob
447fffc8differs from HEAD blobbad31df4.Leg 1 — the src pin, no rebuild needed (the pin imports
./indexfromsrc; the mutation cannot reachpackages/types/dist, which is where itsDataSourcecomes from):invalidateCachetwice, with and without an argument)._HasHiddenMembersand_IsTheAdapterassertions._NotOnDataSource) and 135 (_StillADataSource) produced ZERO diagnostics — the split the pin was designed for.viteston the same mutated tree still passes, which is the point: esbuild erases types, so the colour comes fromtscand from nowhere else.Leg 2 — the dist pin, rebuilt in both directions. Rebuilt from the mutated source, then a dist preflight: the widened marker count in
dist/index.d.tswas 0 (the mutation reacheddist). The standalone dist probe then reproduced the card's failure exactly — 9 x TS2339 — andcheck:doc-snippetswent to exit 1, "455 of 455 block(s) judged, 4 failed": precisely the four README sections this PR rewired, failing ongetCacheStats,invalidateCachex2,clearCache,onConnectionStateChange,getConnectionState,isConnected,onBatchProgress,clearCache,invalidateCache.Restore, proven by state, both halves. Source: restored blob
bad31df4equals HEAD's blob andgit diff HEADis 0 bytes.dist: rebuilt again, widened marker back to 1 and narrow marker 0, dist probe back to 0 diagnostics,check:doc-snippetsback to exit 0, 0 failed. The restore leg is rebuilt on purpose — a mutated marker left indistwould keep acting on every later run in this tree.Clause ②
Clause-②: yes, declared in the claim comment on #7323 in the fixed machine spelling, andneeds:contract-reviewis on both carriers (this PR and the card). It widens the declared public surface reachable through a published entry point. Q1's answer removes the triage's specific worry — no class is exported by implication — but the declaration limb is judged from content, not from paths or from diff size.The one compatibility note a reviewer should weigh. The widened return is a class with private members, so it is no longer satisfied by a hand-written structural stand-in: an object literal assigned to the factory's
ReturnTypewill now be rejected where it used to be accepted. Nothing in this repo does that (searched every test and app that names the factory —apps/console/src/dataSource.tsonly re-exports it), and the fix for a downstream consumer is to annotate such a fake asDataSource, which is what it was standing in for. It is the only direction in which this change is not purely additive, and it belongs in the contract review rather than in a footnote.Draft on purpose, not flipped ready, auto-merge not enabled — the seat lands it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01EMrWaQw3XS5DxTHxp4yRyC
Generated by Claude Code