Uh oh!
There was an error while loading. Please reload this page.
perf(plugin-map): key ObjectMap's dataConfig memo on [schema], not a per-render JSON.stringify - #6266
Merged
Merged
Conversation
…per-render JSON.stringify `getDataConfig(schema)` was called bare in the render body and its result re-serialized with `JSON.stringify` on every render, only to hand back the object the memo already held. `getDataConfig` is a pure function of `schema` (it reads exactly `data`, `staticData`, `objectName`), so `[schema]` gives the same stable identity the fetch effect needs with no serialize and no per-render rebuild — the shape #5976 landed one line below for `mapConfig`. Dropping the serialize is also a correctness move. `JSON.stringify` throws on a value it cannot serialize, and the passthrough branch returns the author's own `schema.data` verbatim, inline rows included — so a record graph with a back-reference took the whole map subtree down from the render body. Part of #6018 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L
… fix Part of #6018 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L
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
|
yinlianghui
marked this pull request as ready for review
August 25, 2026 06:24
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#6018
getDataConfig(schema)was called bare in the render body and its result re-serialized withJSON.stringifyon every render, only to hand back the object the memo already held:getDataConfigis a pure function ofschema— it reads exactlydata,staticData,objectName, nothing ambient — souseMemo(() => getDataConfig(schema), [schema])gives the same stable identity the fetch effect needs, with no serialize and no per-render rebuild. That is the shape #5976 landed one line below formapConfig; the two lines now agree, andmapConfig's docblock (which narrated theJSON.stringifydep key as a live neighbour) was updated so it does not describe a line that no longer exists.The hazard the old comment recorded is real, and was checked rather than assumed
dataConfigis a dependency of the fetch effect and that effect callssetData, so a fresh identity there is a refetch loop rather than mere waste. The loop path is specifically a re-renderObjectMapcauses itself (data landing, the object definition landing, search typing, zoom, selection, geolocation) — and on that path theschemaprop is untouched by construction, so[schema]closes it. Pinned by two tests that drive exactly those renders.Which assertions survive a revert (measured, not asserted)
Measured twice: once against pristine
origin/mainbefore any edit, and once as an explicit revert leg on the committed tree (mutation proven on disk by grepping the injected and the separately removed text; restored undertrap … EXIT INT TERMwith a cwd-independentgit -C … checkout,git diff HEAD --statconfirmed empty afterwards).packages/plugin-map/src/ObjectMap.dataConfigMemo.test.tsx, 4 tests — 3 survive a revert, 1 is red before the fix:[schema]schemaschemachangesThe three identity-contract tests passing on both forms is the honest result and is what the issue predicted: the stringify key really did buy a stable identity, so #6018 is a cost card, not a correctness card. Their value is as regression pins — they fail if this memo is ever re-keyed on something render-fresh again, which is precisely the #5976 defect one line up. They are also the "verify the fetch-effect identity contract against the existing tests" half of the ruling, discharged.
The fourth is the red direction, and it is a genuine correctness gain rather than a timing proxy.
JSON.stringifyis not a total function — it throws on a value it cannot serialize — andgetDataConfig's passthrough branch returns the author's ownschema.dataobject verbatim, inlinevaluerows included. So a map handed inline records carrying a back-reference (an$expand-ed lookup) threw from the render body and took the whole subtree down:Comparing identities never serializes, so the config no longer has to be serializable at all. A
BigIntid is the same class of input.No assertion here counts renders as a proxy for speed. The observable is how many times the fetch effect fired, read at the module boundary it crosses (
dataSource.find), and every stability assertion is paired with a counter-probe — "stable identity" is equally satisfiable by freezing a stale config forever, which would be the worse bug.useMemois not a semantic guarantee — and here something does depend on itStated explicitly because it is load-bearing: React may discard a
useMemocache, and if it discards this one the fetch effect sees a freshdataConfigand re-fires. That is a genuine behaviour dependency, not merely a performance one —setDatare-renders, which is what made "prevent infinite loops" the original comment.This PR does not change that exposure: both the old and the new form are a
useMemo, and the new one is strictly cheaper and no less safe. But it is not fixed here either. A form that does not depend on the cache holding would key the effect on the primitive values it actually reads (dataConfig.provider,dataConfig.object) instead of on an object identity at all — a different and larger change, out of scope for this card and deliberately not smuggled in.Premise fork: is
schemaidentity itself unstable at some call site?Checked before applying the memo, because that is the reason someone reached for
stringifyin the first place. Measured with a throwaway probe component registered through the realSchemaRendererpath (probe deleted; not committed) — it recorded theschemaidentity actually reaching a leaf renderer across a parent re-render:stable: trueresponsiveStyles→stable: falseSo there is a render-fresh
schema, and it isSchemaRenderer.tsx:1076:unmemoized, so any node carrying
responsiveStyles(ADR-0065 scoped styling) gets a fresh object on everySchemaRendererrender. Reported rather than worked around, per the ruling — no identity hack was swapped in for the removed one. Three things bound it:ObjectMaprender.setDatare-rendersObjectMaponly;SchemaRendererdoes not re-render, so the identity holds across exactly the renders the loop would need. Bounded extra fetch, not an unbounded loop.mapConfigon the adjacent line has been keyed[schema]sinceObjectMap's markeruseMemonever memoizes —getMapConfigruns unmemoized in the render body, somapConfighas a fresh identity every render #5976 and is subject to the identical instability, as is the whole marker cascade downstream of it.packages/react, which is outside this card's collision bounds. Recorded in the structured report for the PM to file and grade.The other two callers were re-verified stable on current
main:useElementDataSourceSchema'smappedis aReact.useMemothat returnsschemaby identity when there is no composed binding, andListView'sviewComponentSchemais aReact.useMemo.Verification
All run from the repo root (the canonical invocation —
pnpm --filter … exec vitestand package-cwd runs hit the #3378 guard), and re-run on the final commit8311dbb0e:Dependency closure built first (
pnpm --workspace-concurrency=2 --filter '@object-ui/plugin-map^...' build) so the type-check read rebuiltdist/*.d.tsrather than stale siblings.Repo-wide lint across the farm is left to CI. The local run was narrowed to the changed package and the narrowing is measured, not assumed: the file population came from eslint's own config resolution (
eslint .inpackages/plugin-map→ 23 files, counted from--format json), andeslint.config.jsdeclares noproject/projectService, so type-aware linting is off and every rule is single-file AST — this diff cannot move the verdict on any file it does not touch.Downstream consumers (prefix form,
pnpm --filter '...@object-ui/plugin-map'):@object-ui/console,@object-ui/site,examples/schema-catalog— all apps/examples. No exported surface moved; the change is a private render-body memo.Scope
packages/plugin-maponly, plus the changeset. #5977 (ObjectMap.listViewMapConfigReach.test.tsxstale narration) is open in the same area and is not addressed here — it is a different file and was not touched; no overlap to report.Generated by Claude Code