mui-toolpad
mui-toolpad copied to clipboard
[RFC] Immutability helpers
What's the problem? 🤔
All Toolpad state is immutable. The state is large, we often need to update deeply nested immutable objects.
Example from https://github.com/mui/mui-toolpad/blob/300bf3cc93fbe928db2edfca0284ba7071c05c13/packages/toolpad-app/src/appDom.ts#L561
return update(dom, {
nodes: update(dom.nodes, {
[node.id]: update(dom.nodes[node.id], {
[namespace]: updateOrCreate((dom.nodes[node.id] as Node)[namespace], {
[prop]: value,
} as any) as Partial<Node[Namespace]>,
} as Partial<Node>),
}),
});
One could argue to use plain javascript and spread:
return {
...dom,
nodes: {
...dom.nodes,
[node.id]: {
...dom.nodes[node.id],
[namespace]: {
...(dom.nodes[node.id][namespace] ?? {}),
[prop]: value,
}
}
}
}
However, this doesn't account for the fact that an update (or part of the update) may be a no-op, and trigger unnecessary renders. The impact of this can be profound. e.g. imagine updating the hovered element on every mouse hover event, even if it largely stays the same for many of the invocations.
We currently solved it with a lightweight immutability helper library. But it's starting to show issues scaling, it's verbose and requires discipline to use it throughout the whole deep update.
What are the requirements? ❓
- immutable
- terse
- easy to understand
- reduce the opportunity to introduce errors
- reduce amount of state updates (or reduce the chance of a seemingly innocent change introducing state updates)
- well supported
- type safe
What are our options? 💡
+Compact+Works well with the mental model of mutable code. Essentially you're just writing the exact same code you'd write for mutable objects.+The library is used at one place, then steps out of the way-It's not immediately clear at what the library does for you under the hood-Very shallow learning curve
produce(dom, draft => {
draft.nodes[node.id][namespace] ??= {}
draft.nodes[node.id][namespace][prop] = value
})
const [input, setInput] = React.useState({ nested: { value: 'foo', hello: 1 } })
// ...
setInput(produce(existing => {
existing.nested.value = 'bar'
}))
+Tersest+FP-Very steep learning curve-Hard to understand at a glance
R.set(R.lensPath(['nodes', node.id, 'namespace', prop]), 120000, dom)
update(myData, {
nodes: {
[node.id]: {
[namespace]: {
[prop]: {
$set: value,
},
},
},
},
});
Proposed solution 🟢
The proposal would be to opt for using immer which reduces that code to the following:
return produce(dom, draft => {
draft.nodes[node.id][namespace] ??= {}
draft.nodes[node.id][namespace][prop] = value
})
Relevant resources/benchmarks 🔗
No response
I see no drawback to using a third-party library. Using our own methods hadn't been a pain point for me thus far, but it does have all the drawbacks you mentioned which I agree with.
Like I said, I'm familiar with ramda which just has many helpful methods, but I'm in fine with trying out the latest coolest thing, so if that's immer we can try it. It does seem a bit unfamiliar to me at first glance, maybe a bit more verbose/complex than ramda - or maybe that makes it more flexible.
Another example: https://github.com/mui/mui-toolpad/pull/778/files#diff-e235aa0e73a44734157798060c9962f5b14ae8ecee7e1a295bbb428c9210f41bR58
These are updates to a deeply nested structure that also depend on actual values within. Here it was just easier to deeply clone the object and do the changes imperatively. immer would allow using the exact same code, but avoids the deep clone when unnecessary:
function encryptSecrets(dom: appDom.AppDom): appDom.AppDom {
return produce(dom, (draft) => {
for (const node of Object.values(draft.nodes)) {
const namespaces = omit(node, ...appDom.RESERVED_NODE_PROPERTIES);
for (const namespace of Object.values(namespaces)) {
for (const value of Object.values(namespace)) {
if (value.type === 'secret') {
const serialized = value.value === undefined ? '' : JSON.stringify(value.value);
value.value = encryptSecret(serialized);
}
}
}
}
});
}
I'd vote for immer imo works pretty well, it also has hooks ready to be used: https://github.com/immerjs/use-immer
As a side effect that I mentioned the other day - we could get undo/redo functionality almost for free with immer
As a side effect that I mentioned the other day - we could get undo/redo functionality almost for free with immer
Just to note: We already have immutable application state, immer is not at all unlocking anything new here.
Undo/redo for free is too optimistic. It would indeed be easy for simple applications. But for Toolpad it would not work as expected. one example:
- add component to page
- open another page and add a component
- go back to the page and add another component
undo, undo, undo would visually remove both components from the page you're on, but also on the page you're not on. Which is unexpected. If you're in the page editor you'd only expect to undo things in that canvas, not in other pages or code components, or other parts of Toolpad. (There's several scenarios like that that one can come up with)
Undo/redo for free is too optimistic
yes, that's true. And it's also true that some actions might require multiple undos, however, even if that's the case, that would still be for free. I wasn't saying that we can get free undo/redo which is perfect 😅
I'd rather use undo/redo with subpar experience, than not have it at all 🤷
-edit: anyways, I'd still consider trying out "free" if possible and then see how bad it is before rejecting
undo/redo would have other issues to solve like for example the fact that in the editor sometimes we automatically create or remove rows and columns. maybe something like being able to "group" editor actions would be a good way to solve it - anyway that's not directly related with this issue, just some more obstacles that a "free redo" might have at first.