perf(vue): keep the state of a form shallow - #2623
KiaraGrouwstra wants to merge 1 commit into
Conversation
✅ Deploy Preview for jsonforms-examples ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
coverage: 84.289%. remained the same — KiaraGrouwstra:vue-raw-schema into eclipsesource:master |
EclipseSourceAI
left a comment
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
What this PR does
Adds an asRaw helper in packages/vue/src/components/JsonForms.vue that runs toRaw then markRaw on the schema and UI schema, applied to the initial values in data() and to the new values in the schema / uischema watchers. This keeps the (UI) schema out of Vue's reactive graph so no proxies are created for the thousands of nested schema node reads that testers, resolveSchema and scope resolution perform. Four unit tests are added.
The approach is sound and consistent with how this repo already deals with the problem (markRaw on ajv, i18n, middleware, renderers at various call sites). Marking the root object rather than using shallowRef is the right call, since jsonforms.core.schema and the provide()d state all need the raw object. I checked the Vue packages for in-place schema or uischema mutation and found none, so nothing in this repo regresses.
Critical points for a maintainer
markRawmutates a prop object the library does not own. The__v_skipflag is permanent and global. An app that hands JSON Forms a plain schema and later puts that same object intoreactive()/ref()loses reactivity silently.- The behavior change is broader than the PR text claims. In-place schema mutations do currently refresh renderer-level props (labels, descriptions) because
mapStateToControlPropsreads deep into the schema inside a computed. This warrants aMIGRATION.mdentry for 3.9. - Test gap: every new test passes a plain object, so the
toRawhalf ofasRaw(the documented common case) is never exercised.
Not verified
The sandbox for this review has no network access and no installed dependencies, so neither the unit tests nor the Netlify deploy preview could be exercised. The runtime schema/uischema swap behavior in the vue-vuetify example app (Monaco editor plus apply) has not been checked visually.
| */ | ||
| function asRaw<T>(schema: T): T { | ||
| if (schema !== null && typeof schema === 'object') { | ||
| return markRaw(toRaw(schema) as object) as T; |
There was a problem hiding this comment.
markRaw writes a non-removable __v_skip flag onto the caller's object, which JSON Forms does not own. An app that passes a plain schema here and later puts that same object into reactive() or ref() (schema editor with live preview) silently gets a non-reactive object back. Every existing markRaw in this repo sits at a call site that owns the object (example), so a maintainer should sign off on mutating a prop value here.
There was a problem hiding this comment.
Generally speaking, the AI is right here with that we should never modify a user object. Looking at the code we already do that already in a Vuetify component and should remove it there.
Regarding the PR: I think we should not play around with markRaw/toRaw but instead use shallowRef consistently for all of these large objects. The JSON Forms architecture, especially the @jsonforms/core utils were designed for shallow references as they were originally developed for React which does not support deep references.
This might expose some rerendering issues which are currently masked by the deep reactivity, but all of these can be fixed.
| /** | ||
| * Keeps a schema out of Vue's reactive graph. | ||
| * | ||
| * JSON Forms only reads the schema and the UI schema, thus Vue does not need | ||
| * to make them reactive. Without this, Vue makes a proxy for each nested | ||
| * schema node when a renderer reads that node for the first time. Testers and | ||
| * scope resolution read the schema many times for each control. Thus these | ||
| * proxies make a large form slow to mount. | ||
| * | ||
| * `toRaw` gets the initial object if the caller keeps the schema in reactive | ||
| * state, which is usual. `markRaw` then prevents a new proxy. |
There was a problem hiding this comment.
17 lines of JSDoc for a 5 line helper, and most of it is PR description material. Trim it to the rule and its consequence, the profiling rationale belongs in the commit message.
There was a problem hiding this comment.
I think this comment is good. We should just not keep the toRaw/markRaw behavior but the comment itself was fine.
f01c131 to
0da2bb9
Compare
KiaraGrouwstra
changed the title
0da2bb9 to
f9efaa3
Compare
KiaraGrouwstra
changed the title
d2163ae to
4d4963a
Compare
`JsonForms` returns `schemaToUse`, `uischemaToUse` and the `jsonforms` object from `data()`, thus Vue makes a deep proxy for the schema, the UI schema, the renderers, the cells and `ajv`. Testers and the scope resolution read the schema many times for each control, thus a large form makes and reads many thousands of proxies. A profile of a 160-field form gave 45% of the samples to `isEmpty` and 15% to the proxy `ownKeys` trap. The mount time of that form goes from 1790 ms to 1000 ms. `shallowRef` and `shallowReactive` keep this state shallow. Vue keeps a `shallowReactive` object as it is when it makes the object of `data()` reactive, and the reactive traps read and write through a `shallowRef`, thus the component keeps its usual behavior. A reducer replaces `core` for each change, thus shallow tracking is sufficient for the renderers, which read the state through computed props. The state takes the value of a prop as it is. JSON Forms thus writes no flag on the object of the application, and a deep watcher of the application still runs. An application that keeps such a value in `reactive()` state gives a proxy of the value to the form. `ajv` is the one exception, for which `toRaw` gets the instance itself, because Ajv cannot compile a schema through a proxy of itself. An application that keeps these values out of deep reactive state no longer needs `markRaw` for them. `AdditionalProperties` of `@jsonforms/vue-vuetify`, the example application and the tests no longer apply it. `MIGRATION.md` gets an entry. The form data that a renderer gets is no longer a proxy, thus a change of the data needs `handleChange`. Assisted-by: Claude:claude-fable-5-1 Claude-Session: https://claude.ai/code/session_016KGK8Duuq2tHJMm2mVcdFu
4d4963a to
2c1b22a
Compare
4 participants
Problem
JsonFormsis an Options API component that returnsschemaToUse,uischemaToUseand thejsonformsobject fromdata(). Vue appliesreactive()to the object thatdata()returns, thus the schema, the UI schema, the renderers, the cells andajvall become deeply reactive. Vue makes a proxy for each nested node at the first read of that node.Testers,
resolveSchemaand the scope resolution read the schema many times for each control. A large form thus makes and reads many thousands of proxies.In a profile of a 160-field form,
isEmptywas 45% of the samples and the proxyownKeystrap was 15%.Reactive renderer entries additionally give Vue's "received a Component that was made a reactive object" warning, thus every caller has to apply
markRawto its renderer set by hand.Change
JsonFormskeeps its own state shallow.shallowRefholdsschemaToUse,dataToUseanduischemaToUse. The reactivegettrap unwraps a ref without a conversion and thesettrap writes through it, thus a read gives the raw value and an assignment still notifies the dependents.shallowReactiveholds thejsonformsobject.reactive()returns such an object as it is, thus the object stays shallow inside the state ofdata().provide()gives this same object to the descendants.toRawfor theschema, theuischema, therenderersand thecells.ajvis the one exception, for whichtoRawgets the instance itself. Ajv de-duplicates the entries of its scope by identity and a proxy gives a new wrapper for each read, thusajv.compilewrites a duplicate identifier and throws aSyntaxError.A reducer replaces
jsonforms.corefor each change and never changes it in place, thus shallow tracking is sufficient.useControlandDispatchRendererread the state through computed props, which the replacement ofcorenotifies.On a 160-field form the mount time went from 1790 ms to 1000 ms, with an identical DOM.
Behavior
A new schema, UI schema or renderer set still updates the form. The reactivity stays on the property of the state, and not on the object that the property holds.
An in-place change of these values does not update the form. Renderer props such as labels and descriptions read deep into the schema through
mapStateToControlProps, thus such a change did refresh a part of the UI before. The validation did not run again for it, thus the form was in an inconsistent state. This is not a documented behavior and gets no migration entry.The form data is a sibling of the schema on the
coreobject that the reducer replaces, thus the data is no longer a deep proxy as well. A change of the data needs adispatchof anupdateaction, whichuseJsonFormsControland its relatives do throughhandleChange. A custom renderer that writes into the data object directly, for example with av-modelon a nested field, must usehandleChangeinstead. The migration entry states this.The state is the state of JSON Forms, thus JSON Forms writes no flag on the object that the application supplies. A deep watcher of the application on its own schema still runs, and the application can put that same object into
reactive()elsewhere.An application that keeps these values out of deep reactive state no longer needs
markRaw.AdditionalPropertiesof@jsonforms/vue-vuetifygavei18n,middlewareandajvto a nested form with it, which wrote such a flag onto objects of the application. The example application and the tests applied it to their renderer set and to theirajv. There is nomarkRawleft in the packages.An application that keeps such a value in
reactive()state gives a proxy of the value to the form, because Vue keeps the props of a component shallow. A proxied renderer set gives the Vue warning again, thus such an application keepsmarkRawfor it, or holds the value inshallowReforshallowReactivestate. The migration entry states this as well.findUISchemareturnscontrol.options.detailby reference, thus an inline UI schema below a control is raw as well. This is consistent with the rest of the change.uischemaskeeps its current behavior.Tests
packages/vue/tests/unit/JsonForms.spec.tsgets nine tests.@vue/test-utilskeeps the props of a mount inreactive(), thus the tests mount the form below a parent that holds its state inshallowReactive, as an application does.Two tests check that the given and the generated schema and UI schema stay out of the reactive graph, and that a renderer gets the original nested nodes. Two check that a new
schemaoruischemaprop reaches the core state and that the validation runs again for it. Three check the renderers, the cells and both the given and the generatedajv. One checks that a proxy of an application reaches the core state as theajvinstance itself. One checks that the object of the application gets no flag and that a deep watcher of the application still runs.packages/vue: 12 tests pass.packages/vue-vuetify: 95 tests pass.packages/vue-vanillafails ondev/components/App.vue, before and after this change alike.Assisted-by: Claude:claude-fable-5-1