Keep key on inlined component

[calebmer]

Fixes #2322. The React Compiler was discarding keys for components that were inlined.

Consider:

Input

const React = require("react");

__evaluatePureFunction(() => {
  function Lambda(props) {
    return [<div key="0" />, <Omega key="1" />, <Omega key="2" />];
  }

  function Omega(props) {
    return <div />;
  }

  __optimizeReactComponentTree(Lambda);

  module.exports = Lambda;
});

Output

(function() {
  var _2 = function(props, context) {
    _4 === void 0 && $f_0();
    return [_4, _7, _7]; // <------------------------- `_7` has no `key`!
  };

  var $f_0 = function() {
    _4 = <div key="0" />;
    _7 = <div />;
  };

  var _4;

  var _7;

  module.exports = _2;
})();

---

Cases where this can be an issue:

• First render mode and we need to hydrate instances based on keys (will this be a thing?)
• Compiled component is re-rendered and a list has a different order. List item components are expensive.
• We inline components with state.

Of course, it’s possible this may be a non-issue.

My solution for this was to wrap the inlined React Element in a <React.Fragment> with a key. So my input above becomes:

[
  <div key="0" />,
  <React.Fragment key="1"><div /></React.Fragment>,
  <React.Fragment key="2"><div /></React.Fragment>,
]

Cloning inlined elements and adding keys is a fragile operation since the element may already have a key or the element may be some other React node like a portal. I opted for wrapping in <React.Fragment>. Note that this also means we can hoist <div /> in the example above.

Thoughts on adding an optimization which clones the element and adds key={x} in cases where this is possible? <React.Fragment> seems correct in all cases, but cloning with key={x} when possible seems like it might be faster.

Also happy to hear other suggestions for maintaining the key on inlined elements.

This has no impact on our internal bundle.

[trueadm]

This looks good in principle – you will need to update the React test snapshot (use the -u flag). We already have logic that applies keys in branching.js but I like your approach here in use fragments. We do create more unnecessary nodes that required, but given they're fragments, it shouldn't be a big deal.

[trueadm]

Can we extract this key adding logic into its own function that has a descriptive name. We might want to apply the same logic in other parts of the reconciler or in branching.js at some point so it would be useful to have as utility function.

[gaearon]

Keys shouldn't matter for first render mode, but matter in normal mode.

[calebmer]

Keys shouldn't matter for first render mode, but matter in normal mode.

@gaearon could they matter when we hydrate the first-render tree? For instance if we bail out on a class component. Or is my mental model of hydration wrong?

[trueadm]

@calebmer Hydration isn't affected by keys. Furthermore, we never render any key information or metadata during server-side render, so the hydration process always mounts components when traversing the SSR tree.

[calebmer]

@gaearon disabled this for first render mode 👍

[facebook-github-bot]

calebmer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[gaearon]

I don't quite understand the discussion about hydration in this thread.

SSR works in two steps:

1. First, HTML is produced. That usually happens on the server although technically you can run it on the client too. When you produce HTML, keys don't matter because there are never any updates. You can produce HTML both with normal bundle and with first-render-only bundle. In fact "first-render-only" bundle is exactly "the subset of the normal bundle that is sufficient to produce the same HTML with less code".

2. Secondly, DOM is hydrated. This always happens on the client. Hydration happens the same way a normal initial client render works, except that we reuse existing DOM nodes as we traverse them instead of creating new ones. Hydration can only be done with a "full" bundle, because "first-render-only" bundle would neither have events nor lifecycles nor support for updates. The whole point of hydration is to attach event handlers (which can trigger updates). In a full bundle, keys matter — if the keys are wrong, the first render (hydration itself) would work but any updates would potentially work differently.

An ideal end state is Prepack being able to produce two bundles. One for the initial render, and the other one being "just enough to support updates without repeating the initial render logic". However this would require React to have a way to "progressively enhance" components with update logic after they've already rendered or been hydrated. Some future things we talked about can help us with that. I'm not sure how we'd deal with keys in this scenario but they would probably have to be specified in the initial render bundle so that the first update is hooked up correctly. So maybe we're really talking about three bundles: one for SSR (no keys), one for first render (with keys), and one for updates. But that's a far future ideal, and not what we do today.