fix(core): serialize the 'key' attribute of JSX nodes

[yanivhamo]

Added serialization and deserialization of the 'key' attribute, the only member of JSXNodeImpl which was not serialized thus far.

Issue #5496

[netlify]

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	f428520

[yanivhamo]

Note that the 'key' attribute is populated by some value whenever it is not explicitely set, e.g. '81_1'. Therefor with this change we can expect an increase in byte size of serialized nodes whether or not they had their 'key' attribute set explicitely. However I don't see a way around paying this price, if we want perfect serialization/resumption.

[wmertens]

We could also check that the key is a short number or string that doesn't need serialization? The dev can choose it.

I don't see why we should allow arbitrary keys. Let's make it max 16 characters?

[yanivhamo]

The thing with keys, is that they must be unique inside a list in order for Qwik/React to do their thing; not just unique, but uniquely identifying the specific element they are set on, or else we break in run time. A restriction on length could force the developer to be overly conscious when choosing keys, I don't think we want that. The magic of resumption is appealing precisely because it is as seemless as possible.

A possible solution would be to somehow mark keys automatically-generated by the framework, from keys set by the user. The former could be serialized to some special single character that when read back, is replaced with an automatically generated key in runtime. These auto keys tend to be 4-5 chars long, and we'll get some saving by replacing them with a single magic character. However I am not sure what is the actual gain we would observe over the wire, after compression does its thing, so I cannot gauge whether or not it is worth the added complexity in code.

[PatrickJS]

@yanivhamo can you update your PR?

[wmertens]

[wmertens]

@yanivhamo actually I just realized - I'm pretty sure that when the keys are auto-generated, they are the same on server and client, and therefore they don't need serializing, right?

So, can we handle that case? I think it needs an extra flag on the JSXNode.

[yanivhamo]

Let's do this! @wmertens can you please point me to the general area of where keys are being auto-generated? For the life of me, I can't find it.

[wmertens]

@yanivhamo That's the spirit 😎

I'm not so sure myself - here's what seems to be the code for CSR:
https://github.com/BuilderIO/qwik/blob/123c2bdae96a369ec386179c7316ad021f85b78e/packages/qwik/src/core/render/jsx/jsx-runtime.ts#L236

But I think maybe the SSR side doesn't use keys at all?

The way to tell would be to look at SSR output of your branch and see what it makes of it

[yanivhamo]

@wmertens Yeah that line executes only if the key is already set. The key (no pun intended) to understanding what's happing here is that all "generated keys" end up being the same symbol. That means that there's no "smart" logic on the server that understands this is a key that must be unique. It looks like something is merely mapping values into hashes(?) and it simply maps all null values (for unset keys) into the same symbol.
I'm looking into this.

[yanivhamo]

Thinking about it more, I don't think the issue is with serialization.

When we create a JSXNode without key on the server, it gets some key associated with it already on the server's side, before serialization. E.g. this code, part of my example:

  const list = [1,2,3].map(idx => <Item index={idx}/>);
  console.log(list.map(it => it.key).join(","));

Logs 81_1,81_1,81_1 on the server's console.

This PR serializes this assigned key to the client, such that the client has exactly the same keys as on the server. I argue now that this is the correct behavior. It would be wrong from a maintenance point of view to re-generate automatic keys on the client and hope that they would match the ones generated on the server. Much cleaner to correctly serialize everything that the server sets.

The logic of serializing keys is therefor correct. Now, one can argue, that the server shouldn't be generating keys for JSXNodes on its own, and surely not in the way it is doing so now, where it assigned the same key to all elements. But this has nothing to do with serialization, it would affect code accessing the key on the server side. It is therefor something we should do in a separate PR.

@wmertens what do you think?

[wmertens]

@yanivhamo I'd like to merge it but it needs rebasing