Repository navigation
refactor(core): derive DatasetResultField from spec AnalyticsResult.fields[] - #4128
Merged
Merged
Conversation
…ields[] (#3815) `packages/core/src/utils/dataset-format.ts` declared its own six-key interface for the analytics result column under a doc comment describing the server's contract. The key set matched the spec today, so nothing was broken — but this was the last surviving member of the derive-don't-restate family (#3613/#3753 parameter side, #3752 adapter return side), and the member with no compile-time tripwire: three surfaces (DatasetWidget, DatasetReportRenderer, DatasetPreview) consume this name AS the real column type, so the next spec column key would simply never appear here and no build would complain. It is now `AnalyticsResult['fields'][number]`. The exported name is unchanged and the `PercentScale` re-export is untouched. `type` is required again, as the contract has it. The investigation the card asked for found no construction site that could not supply it: every value of this type originates in `ObjectStackAdapter.queryDataset`, which already declares the spec element (#3752), and the whole downstream type-check closure of `@object-ui/core` (68 tasks) is green with no fixture change. No consumer reads `.type` at all, so the relaxation had bought no caller anything while advertising a `string | undefined` the wire never produces. `packages/core/tsconfig.typetests.json`, chained off the package's `type-check`, compiles the new parity test — core's test tree is still in the type-check coverage script's TEST_DEBT, so without it these pins would be the "declared != enforced" landmine they exist to remove (objectui#3009). Reverting the interface turns five of them red, including the negative pin against the hand-written shape and the `ChartResultField` superset asymmetry. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
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.
Fixes #3815
Premise check (first, per the card)
Both halves re-verified before implementing, on current
origin/main(cfeb378b5) and the resolved installed@objectstack/spec@17.0.0-rc.5(read out ofnode_modules/.pnpm, not framework main):packages/core/src/utils/dataset-format.tshas not moved since2a54e860c; the hand-writtenDatasetResultFieldinterface is at line 28 withtype?: string.AnalyticsResultindist/contracts/index.d.tsdeclaresfieldsas an inline array element with the same six keys andtype: stringrequired. There is no named exported element type for it, soAnalyticsResult['fields'][number]is the only derivation available.Premise holds: same key set, one relaxation (
type), and no drift today.Phase 1 — what the construction-site read found
The card left two pre-decisions open. Both resolved by reading, and both resolved the same way:
1.
typecan be required. Nothing in this repo constructs a result column. Every value of this type originates inObjectStackAdapter.queryDataset(packages/data-objectstack/src/index.ts:3322), whosefieldswas already declared asArray< AnalyticsResult['fields'][number] >by #3752 — so the producer has been handing out required-typecolumns all along, and the consumer type was the looser of the pair. The three named consumers only ever receive:DatasetWidget(fields?: DatasetResultField[]state),DatasetReportRenderer(same),DatasetPreview(same). No adapter or mapper constructs one either —queryDatasethas no other implementation inpackages/,apps/orexamples/. The only fixture typed asDatasetResultField[](dataset-format.test.ts:86) already suppliestypeon both entries.2. Nothing reads
.type. A sweep of the dataset surfaces for a read off a result column found zero. Every.typehit in those files belongs to a different shape (widget.type,chart.type,report.type, dashboard filter defs,ReportViewer's spec-report columns). So the relaxation had bought no caller anything, while advertising astring | undefinedthe wire never produces.That is the PREFERRED path in the dispatch, so no fallback tripwire was needed: full derivation.
What changed
Public name preserved; the
PercentScalere-export at line 51 is untouched, so existing@object-ui/coreimport paths keep working. The doc comment no longer re-lists the keys — re-listing them is the failure mode #3752 named.ChartResultFieldclaim: verified, and it survives the tightening.ChartResultField(chart-series.ts, renderer-internal, nothing in the spec owns it) needs onlyname/label?/format?. EveryDatasetResultFieldis still one, which is load-bearing:DatasetWidget:1050hands itsDatasetResultField[]state straight tobuildChartSeries. The claim is now pinned rather than asserted in prose. One genuinely new fact, also pinned: the reverse direction stops holding — withtyperequired, a bare{ name, label?, format? }no longer satisfiesDatasetResultField. No caller flows that way, but that asymmetry is the entire content of the word "superset", so it is recorded instead of assumed.Where the pin lives, and why it needed a tsconfig
@object-ui/coreis inscripts/check-type-check-coverage.mjs'sTEST_DEBT(72 errors, #4118) and its build tsconfig excludes test files — so notscinvocation reads core's test tree. A compile-timeAssert< Equal< Local, Spec > >dropped in there would have been exactly the "declared != enforced" landmine objectui#3009 found (assertions under a header calling them "the real enforcement", with reverting the derived alias producing zero errors).So this PR follows the repo's sanctioned narrow-rescue pattern (objectui#3181, as used by
auth/components/plugin-detail): apackages/core/tsconfig.typetests.jsonwith an explicit include list (not a glob — the rest of the debt tree must stay out), chained from the package'stype-checkscript, which is what CI's Type Check job runs.check-type-check-coverage.mjsenforces that chaining and is green; its TEST_DEBT comment is updated so the "partially covered already" note names core as well as app-shell.Reverse verification (predicted direction: RED)
Took the fix out with
git checkout -- packages/core/src/utils/dataset-format.ts(restoring the exact pre-fix interface — nogit stash), re-ran the typetests project, restored with a saved patch. Five assertions went red, at the predicted lines:Restored, both projects green again.
One honest deviation from the dispatch template. It asked to "demonstrate a consumer reading
field.typeno longer needs undefined-handling (and clean up now-dead undefined guards)". There are none to clean up — no consumer reads.typeat all, which is why the requiredness restoration is free rather than a coincidence. Reporting that instead of manufacturing a guard to delete.Verification
pnpm --filter '@object-ui/core^...' buildthenpnpm --filter '@object-ui/core' type-check(tsc --noEmit && tsc -p tsconfig.typetests.json) — green.--filter '...@object-ui/core'= the packages that depend on core, not its dependencies):turbo run build42/42 tasks,turbo run type-check68/68 tasks green. No construction site anywhere needed to change — which is the measurement behind decision 1 above.vitest run packages/core/ packages/plugin-dashboard/ packages/plugin-report/ packages/data-objectstack/— 139 files, 2357 tests passed.vitest run packages/app-shell/src/views/metadata-admin/previews/ packages/plugin-charts/— 55 files, 570 tests passed.check-changeset-presenceOK,check-changeset-fixed+check-changeset-no-majorOK,check-type-check-coverageOK (now "10 with a narrow type-assertion project"),check-control-bytesOK,check-spec-symbol-derivationOK,eslinton core 0 errors.Changeset
@object-ui/core: minor. Patch was arguable — no runtime change, no in-repo caller affected — but the released.d.tsnarrows: a downstream consumer assigning a column literal withouttypenow fails to compile, and that is what consumer-visible means. AGENTS.md's bump policy reservesmajorfor following@objectstack/specacross a major and directs objectui's own breaking changes tominorwith the breaking semantics spelled out in the body, which the changeset does.Generated by Claude Code