onUpdate: split node-level { value } from whole-document { data } (#375)#380
Merged
Conversation
Bundle size impact
|
| Format | Base raw | PR raw | Δ raw | Base gzip | PR gzip | Δ gzip |
|---|---|---|---|---|---|---|
| esm | 57.38 KB | 57.66 KB | 🔺 +288 B (+0.49%) | 20.48 KB | 20.60 KB | 🔺 +124 B (+0.59%) |
| cjs | 58.87 KB | 59.16 KB | 🔺 +288 B (+0.48%) | 20.52 KB | 20.64 KB | 🔺 +124 B (+0.59%) |
Measured from build/index.{cjs,esm}.js. Gzip at level 9.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes the onUpdate override contract to distinguish between node-level overrides ({ value }, applied at the edited node’s path) and whole-document overrides ({ data }, applied at the root), preventing accidental full-document clobbers when callers intended to “tidy” a single edited value.
Changes:
- Update the public
UpdateResulttype to support{ value?: unknown; data?: T; error? }and document the new semantics. - Map
{ data }→ root override and{ value }→ node-path override (edit/add only) inrunUpdate, and carry the overridepaththrough the commit outcome to reconciliation. - Expand Jest coverage to validate node-level vs whole-doc overrides across edit/add/delete, and update docs/demo/migration guidance plus a major changeset.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/JsonEditor.test.tsx | Replaces the prior whole-doc { value } test with coverage for { value } (node-level), { data } (whole-doc), gated events, and both-keys warning. |
| src/types.ts | Updates UpdateResult object form to include { data } and documents event-specific semantics and precedence. |
| src/JsonEditor.tsx | Adjusts runUpdate to resolve override scope and return { status:'override', value, path }, warning when both keys are returned. |
| src/contexts/EditingProvider.tsx | Extends UpdateOutcome to carry an override path and applies overrides via applyValue(outcome.path, ...). |
| README.md | Updates onUpdate return-type docs to explain { value } vs { data } and precedence/warning behavior. |
| migration-guide.md | Updates migration guidance to reflect { data } as the whole-document override key and clarifies the breaking change. |
| dev-docs/MANUAL-TESTING-editing-model.md | Updates manual testing instructions to use { data } for whole-document overrides. |
| dev-docs/EditingModel-new.md | Updates editing model documentation to reflect the new override split and semantics. |
| demo/src/examples/static/on-update/Example.tsx | Updates the on-update demo to show node-level { value } and whole-doc { data } usage. |
| demo/src/examples/editing-model/Example.tsx | Updates the “override” mode example to return { data } instead of { value }. |
| demo/src/demoData/dataDefinitions.tsx | Migrates demo callback sites from { value: newData } whole-doc overrides to { data: newData }. |
| .changeset/onupdate-value-data-split.md | Adds a major changeset describing the breaking change and migration path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Addresses PR review: a `jest.spyOn(console, 'warn')` whose manual `mockRestore()` runs only on success leaks the spy into later tests if an assertion throws first. Enable `restoreMocks: true` globally so every spy auto-restores after each test, and drop the now-redundant manual restores (in the both-keys test and the pre-existing drag-and-drop key-collision test). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses PR review: clarify that `{ value: undefined }` / `{ data: undefined }`
are deliberately treated as "no override for this key" (consistent with the
protocol's top-level `undefined`/`void` = proceed, and the adjacent `error`
check), rather than overriding a node to `undefined`. Comment only — no
behaviour change.
Co-Authored-By: Claude Opus 4.8 (1M context) <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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #375.
Summary
An
onUpdatethat returns{ value: X }previously replaced the whole document withX(applied at the root) — the name reads like "this node's value", so returning a node value silently clobbered the entire document. This splits the two intents:{ value: X }→ the edited node's value, applied at its path. The common "tidy what the user just typed" case (lower-case, round, trim, sort this array). Honoured foredit/add; silently ignored forrename/move/delete(no node value to set).{ data: X }→ the whole document (the previous behaviour). Cross-field changes — stamping alastModified, sorting siblings, canonicalising structure. Works on every event.Returning both is a mistake:
datawins,valueis ignored, and aconsole.warnis emitted.How it works
applyValue(path, value)already existed — node-level is justapplyValue(nodePath, value), whole-doc isapplyValue([], data). The result-mapping inrunUpdateresolves which path to use (it has the event + node in scope; foradd,input.pathis the new child's full path), theoverrideoutcome carries that path, andreconcileapplies it.src/types.ts—UpdateResultobject form is now{ value?: unknown; data?: T; error? }.src/JsonEditor.tsx—runUpdatemapsdata→root /value→node (gated to edit/add) and warns on both keys.src/contexts/EditingProvider.tsx—overrideoutcome carriespath;reconcileapplies at it.Breaking change
{ value }no longer replaces the whole document. AnyonUpdaterelying on that (whole-document timestamp/sort transforms) must switch to{ data }. v2 is in beta, so now is the time. Migration-guide §9 updated;majorchangeset added.Tests
Rewrote the old whole-doc test and added 6 to
test/JsonEditor.test.tsxcovering the new contract: node-level edit (proves sibling preservation, not a root replace),{ data }whole-doc, value-on-add, value-ignored-on-delete, data-on-delete, and both-keys-warn. All 6 were confirmed to fail for the right reason before the fix, green after.Docs & demo
dev-docs/EditingModel-new.md+MANUAL-TESTING-editing-model.md.{ value }sites to{ data }(on-update example — which also gained a node-level{ value }showcase + a new{ data }branch; editing-model "Override" mode; three in the liveData guestbook). Removed theTODO(#375).Verification
pnpm test(670 pass),pnpm compile,pnpm lintall green; demo typechecks against the updated core.🤖 Generated with Claude Code