[Idea] Disable read-only proxy in SSR v2

[nolanlawson]

SSR v2 (and v1, for that matter) makes use of getReadOnlyProxy from observable-membrane to ensure that child components do not try to mutate their parent components' props.

@api props
connectedCallback() {
  this.props.foo = 'bar' // throws in SSR
}

Note

Note this is not exactly how engine-dom works – in that world, a child trying to modify its parent's props ends up doing a no-op. (This is why component authors often do JSON.parse(JSON.stringify()) to clone their props.) The original intention was to avoid two-way data flow – e.g. a parent rendering <div>{props.foo}</div>, where props.foo could have been mutated by a child.

In SSR, the read-only proxy is a nice guardrail. However, it comes with a fairly substantial perf cost: ~44% in one benchmark (PoC: ab3e2f9):

To be fair: there is less risk in SSR of a child truly mutating its parent, since rendering is one-time and top-down. There is still, however, potential for mischief if a child ends up modifying its siblings (e.g. because the parent passes in a prop and that prop is shared among siblings).

Thoughts:

• Should we put this proxying behind a flag like process.env.NODE_ENV !== 'production'?
• Should we just remove it entirely?

The downside of removing it entirely is that we would potentially be looser on the server than on the client. This could actually matter for SSR-only cases, where we don't have the extra validation of running components client-side (where children-mutating-parents is already disallowed). But maybe it's still worth it?

[pmdartus]

Keep me honest @nolanlawson, as far as I know, LWC is the only framework enforcing 1-way dataflow using guardrails. I haven't seen any issues related to read-only proxy since LWC's inception. Is it because developers never run into those issues or is it because the guardrails are effective, it's hard to say.

IMO, the CPU and memory overhead aren't worth it in production. I would suggest only enabling it in dev mode.

[pmdartus]

Looking at the client code, it appears that readonly membrane is applied in both dev and production. I was always under the assumption that this check was only enforced in dev mode only (code).

@nolanlawson Is it possible to re-run the same test in the browser to determine what is the runtime overhead with engine-dom?

[nolanlawson]

@pmdartus This topic actually came up when we did a similar optimization for engine-server: #2961 (comment)

Basically:

1. LWC has always enforced read-only access for child components to their props, in both prod and dev mode. This goes back to at least 2020.
2. Are we the only framework that does this? Not sure. I'll have to do a thorough analysis. I did quickly check Svelte though, though, and you're right – children can mutate their parents.

[nolanlawson]

Update on Vue: they claim to have one-way data flow, but they don't enforce this deeply. I managed to get a child to mutate its parent, same as in Svelte.

Is it possible to re-run the same test in the browser to determine what is the runtime overhead with engine-dom?

For the engine-dom side of things: yes, if you comment out this line:

lwc/packages/@lwc/engine-core/src/framework/base-bridge-element.ts

Line 59 in bedc0a0

newValue = getReadOnlyProxy(newValue);

... then you get about a 8-17% improvement in the tablecmp render 10k test:

We can certainly consider relaxing this for both client and server, given the potential perf gains. I'm concerned, though, that it would cause some backwards incompatibilities to do so. Code that was a no-op before now causes the child to mutate its parent's state.

We could also make this an error/warning only in dev mode, but in my experience people ignore whatever we do in dev mode. Many LWC component authors only test in prod mode.

[nolanlawson]

Is it because developers never run into those issues or is it because the guardrails are effective, it's hard to say.

I personally ran into this as a component author before I joined the LWC team, and I found I had to do JSON.parse(JSON.stringify(props)) to avoid my setters becoming a no-op.

[pmdartus]

We could also make this an error/warning only in dev mode, but in my experience people ignore whatever we do in dev mode. Many LWC component authors only test in prod mode.

You are right. This goes back to a larger conversation on how to make warnings more visible to developers. I made piece with the fact that developers don't know/care about console warnings in dev mode.