Repository navigation
[data grid] Fix updateRows stripping class prototypes from rows in datasource mode - #22288
Conversation
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
| // Update the data row lookups. | ||
| dataRowIdToModelLookup[id] = { ...oldRow, ...partialRow }; | ||
| // Preserve prototype of partialRow to support class-based row models | ||
| const merged = Object.create(Object.getPrototypeOf(partialRow)); |
There was a problem hiding this comment.
Codex review:
This fixes the simple public-field repro, but it is not the same as preserving the incoming row instance.
Two gaps remain:
- If a class row uses private fields, prototype getters/methods still throw because
Object.create(Person.prototype)does not brand the object with#privatefields.- If the existing row is a class instance but the update is a documented partial object (
updateRows([{ id, first }])), the new row still getsObject.prototype, so the stored class prototype is lost.
This conflicts withupdateRows()being documented for partial row updates, and with the original row cache preserving input row objects directly (gridRowsUtils.ts:111-114)
So it seems the fix so far only works if every update is a class instance, but it doesn't work for plain partial updates
There was a problem hiding this comment.
One more subtle edge case: If oldRow has a custom prototype but partialRow is a plain object, oldRow's prototype is silently discarded. The fix always uses partialRow's prototype. This is arguably correct semantics (the update dictates the shape of the row), but worth being aware of.
There was a problem hiding this comment.
Addressed in commit c7a8f2b.
Gap 1 (private fields): This cannot be fixed through a spread-merge approach. Private class fields (#field) are brand-checked against the original instance — any Object.create() + spread will throw when those fields are accessed. A code comment is now added explaining this limitation: rows that rely on private fields must supply a fully-constructed class instance as partialRow (rather than a plain-object partial), at which point rule 1 of the new logic takes over and the instance prototype is used directly.
Gap 2 (plain partialRow silently discards oldRow prototype): Fixed. The new logic checks Object.getPrototypeOf(partialRow) === Object.prototype (i.e. "is partialRow a plain object?"). If so, it uses oldRow's prototype for the merged object instead, preserving the class prototype chain of the existing row. If partialRow is itself a class instance, its prototype wins (the caller is passing a fully-constructed instance).
Three regression tests added in rows.DataGrid.test.tsx covering all three cases.
There was a problem hiding this comment.
Addressed in commit c7a8f2b.
The fix now distinguishes between a plain-object partialRow and a class-instance partialRow:
partialRowis plain → useObject.getPrototypeOf(oldRow)for the merged object, sooldRow's class prototype is preserved.partialRowis a class instance → usepartialRow's prototype (caller owns the prototype chain).- Both plain → result is a plain object (no change in behaviour).
Tests added for all three cases in rows.DataGrid.test.tsx.
michelengelen
left a comment
michelengelen
left a comment
There was a problem hiding this comment.
Apart from the mentioned change requests could you please add some tests for the different cases?
| // Update the data row lookups. | ||
| dataRowIdToModelLookup[id] = { ...oldRow, ...partialRow }; | ||
| // Preserve prototype of partialRow to support class-based row models | ||
| const merged = Object.create(Object.getPrototypeOf(partialRow)); |
There was a problem hiding this comment.
One more subtle edge case: If oldRow has a custom prototype but partialRow is a plain object, oldRow's prototype is silently discarded. The fix always uses partialRow's prototype. This is arguably correct semantics (the update dictates the shape of the row), but worth being aware of.
|
@michelengelen, that edge case is worth dwelling on — I wonder if it's actually more central than it looks at first. Would something like this work as a small refinement? Falling back to oldRow's prototype when partialRow is a plain object: const partialProto = Object.getPrototypeOf(partialRow);
const proto =
partialProto === Object.prototype
? Object.getPrototypeOf(oldRow)
: partialProto;
const merged = Object.create(proto);
Object.assign(merged, oldRow, partialRow);That would handle: It still can't disambiguate a cross-class patch (Person → Manager), but that feels exotic enough that "pass a full instance if you mean that" seems reasonable. The deeper concern — also raised by @mj12albert re: private fields — is that any heuristic merge has to guess what the caller meant. That's why in #22282 I suggested an explicit _action: 'replace' branch alongside 'delete': callers who want identity-preserving row swap opt in explicitly, Object.assign is bypassed entirely (so private fields survive), and the default updateRows semantics for partial patches stay exactly as documented. |
|
How can we go forward on this one? |
Hey @jvskriubakken it seems the author is not responding, so we either pick this up ourselves, or if you have some free time please feel free to open up a PR yourself. I'll see if i can pick this up next week. |
|
If I open up a PR my self, do I then open of from this one? Or from scratch from master? |
|
hi, sorry for being offline for some time |
|
Tests added in commit c7a8f2b under a new
Also addressed the two gaps from @mj12albert and the edge case from @michelengelen (inline) — see inline replies for details. |
|
@mixelburg Would you be able to look into the failing build? |
…models
Object spread { ...oldRow, ...partialRow } discards the prototype of
partialRow, breaking class-based row models with prototype getters or
methods. Use Object.create to preserve partialRow's prototype when
merging updates.
Fixes mui#22282
…ject, add tests When `updateRows` is called with a plain-object partial update, the previous fix always used `partialRow`'s prototype (Object.prototype), silently discarding the class prototype of `oldRow`. New logic: - If partialRow is a class instance (non-plain proto), use its prototype — caller owns the prototype chain. - If partialRow is a plain object, use oldRow's prototype so the existing row's class is preserved. - Both plain → result is plain (no-op). Adds a comment explaining that private class fields (#field) cannot be preserved through spread-merge; those rows must supply a fully-constructed instance as partialRow. Adds three regression tests covering: 1. Class instance updated with plain-object partial → prototype preserved from oldRow 2. Plain-object row updated with class-instance partial → prototype from partialRow 3. Plain + plain → result is plain object
c7a8f2b to
eb7b463
Compare
|
Hi! Apologies for the delay. I will look into the failing build and push a fix. Thanks for the ping! |
|
Hi @mj12albert @michelengelen! All review feedback was addressed back in May (commits c7a8f2b and earlier). Is there anything else needed here? |
michelengelen
changed the title
updateRows stripping class prototypes from rows in datasource mode
michelengelen
left a comment
michelengelen
left a comment
There was a problem hiding this comment.
LGTM ... seems the concerns are addressed clearly
|
Seems the only blocking step is CI... could you run |
|
Done! Ran prettier on the test file and pushed the formatting changes. Let's see if CI passes now. |
Fixes #22282
In
dataSourcemode,updateRowsmerges new rows with existing ones using object spread ({ ...oldRow, ...partialRow }), which discards the prototype ofpartialRow. This breaks class-based row models that rely on prototype getters or methods.Changed to
Object.create(Object.getPrototypeOf(partialRow))followed byObject.assignto preserve the prototype of the incoming row while still merging properties from both old and new rows.