fix(ssr): make child props immutable #4779
Conversation
| for (let i = 0; i < obj.length; i++) { | ||
| res[i] = cloneAndDeepFreeze(obj[i]); | ||
| } | ||
| freeze(res); | ||
| return res; | ||
| } else if (!isNull(obj) && isObject(obj)) { | ||
| const res = create(null); | ||
| for (const [key, value] of entries(obj as any)) { | ||
| (res as any)[key] = cloneAndDeepFreeze(value); | ||
| } | ||
| freeze(res); | ||
| return res; |
There was a problem hiding this comment.
Interesting strategy, going for a classic for loop for one but a for...of w/ destructuring for the other.
There was a problem hiding this comment.
Yeah I wasn't really going for consistency. I figure we will have to adjust this anyway because I imagine someone is relying on weird differences between forEach/for loops for hole-y arrays anyway. But I can make it a plain loop in both for now.
| // that they are immutable. This is one of the normal guarantees of both engine-dom and engine-server that we want to | ||
| // emulate in ssr-runtime. The goal here is that a child cannot mutate the props of its parent and thus affect | ||
| // the parent's rendering, which would lead to bidirectional reactivity and mischief. | ||
| export function cloneAndDeepFreeze(obj: any): any { |
There was a problem hiding this comment.
Could something like immer handle this? (Do we have that in runtime or just compiler?)
There was a problem hiding this comment.
I think immer is just in the compiler.
Long-term, we may delete this function entirely. I'm sure it adds perf overhead, and it's arguably not useful since you will likely get an error on the client side if you rely on children mutating their parents' props. Maybe it makes sense in dev-only, or pure-SSR-(no hydration)-only.
Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
Details
This fixes the
parent-child-read-only-propby making props read-only when passed from the parent to the child.Long-term, we can debate if this is worth it or not – it probably adds a perf overhead, but it is technically consistent with the current engine.
I also added a test to ensure this works correctly for props-that-kinda-look-like-attributes (e.g.
title/spellcheck).Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
Gus WI
W-17150218