React Hooks misleading benchmark ?

[WebReflection]

I've been testing neverland against React, as it's fully inspired by its hooks concept. However, if you remove the key={item.id} assignment from this row, to make it non-keyed, the benchmark runs fine only because the removed rows are decreasing, but never increasing.

If you just drop that key and you build-prod and try to remove twice the same row, or row 2 then row 4, you'll have the whole benchmark/browser stuck forever because the logic is incapable to deal with such scenario.

Accordingly, since I believe React with hooks doesn't require mandatory keyed transforms, and apologies if it does as I wasn't aware of it, the current benchmark is misleading, as the same code, or similar up to the keyed point, cannot be used in non-keyed scenarios.

I am not sure this means React with hooks result should be invalidated, as it actually works well when keyed rows are used, but I think I've found a glitch in this benchmark suite, as the removal of rows should rather be both for the previous row, but also for the row after, to bail out all frameworks incapable of dealing with real-world scenarios, instead of having facilitated only the "drop previous row" one, which is currently the case.

Please let me know if you'd like me to file a PR that change the current row removal benchmark, or penalize keyed tests that wouldn't work once non-keyed, or even ignore this issue, as considered not relevant for the current results.

Thank you.

[ryansolid]

I'm not sure we should be that worried about non-keyed or trying to port the keyed implementation to it. I didn't bother when I added the React Hooks implementation, because non-keyed (index as key) is considered generally seen as bad practice. https://reactjs.org/docs/lists-and-keys.html#keys

If a non-keyed implementation doesn't work properly as you described I think it would be better to fix it so it did. Is there a simple test we could do to check that it is working properly as described for the real world scenario? I think it would be helpful to identify any bad implementations. Personally I haven't experienced this but I think that might be because I generally don't contribute non-keyed implementations and tend to ignore the category altogether since it can cause dangerous side-effects.

@krausest My perspective is If it works fine as a keyed implementation then there is nothing wrong with the keyed implementation. These are completely different paradigms and there should be no expectation the same code minus the key attribute should work the same. You build the implementation to fit the task.

[WebReflection]

non-keyed (index as key) is considered generally seen as bad practice

I think non keyed is a way more common pattern than keyed, as you don't always have a unique ID to associate to every rendered component, and while I agree framework incapable of doing non-keyed could be listed under keyed only, and it was the other way around for hyperHTML or lighterhtml implementations, where keyed results were introduced later on as these libraries works more than excellently with non-keyed structures, I think what matters here is that the specific remove row benchmark doesn't reveal the fact some test implementation would fail if the removed row is after, or the same, i.e. removing twice the same visible row, or removing a row that is after a previously removed one.

Let's also remember that all non-keyed results are usually faster than keyed one, but there's currently no way to compare React Hooks performance here, with other libraries capable of doing non-keyed results just fine, because while the test passes, and perf are great, the test doesn't reveal that some non-keyed implementation would actually fail if used in the real-world, as users should be able to remove any row, not just previous rows from a specific starting point.

Moreover, the current keyed tests might not reveal the same issue with other frameworks too, but I haven't manually tested all of them.

Accordingly, I think the remove row benchmakr should be changed to reveal frameworks that would fail at removing rows after the 10th too.

const benchRemove = new class extends Benchmark {
    constructor() {
        super({
            id: "06_remove-one-1k",
            label: "remove row",
            description: "removing one row. (" + config.WARMUP_COUNT + " warmup runs).",
            type: BenchmarkType.CPU
        })
    }
    async init(driver: WebDriver) {
        await testElementLocatedById(driver, "run", SHORT_TIMEOUT);
        await clickElementById(driver, 'run');
        await testElementLocatedByXpath(driver, "//tbody/tr[1]/td[2]/a");
        for (let i = 0; i < config.WARMUP_COUNT; i++) {
            await testTextContains(driver, `//tbody/tr[${config.WARMUP_COUNT - i + 4}]/td[1]`, (config.WARMUP_COUNT - i + 4).toString());
            await clickElementByXPath(driver, `//tbody/tr[${config.WARMUP_COUNT - i + 4}]/td[3]/a/span[1]`);
            await testTextContains(driver, `//tbody/tr[${config.WARMUP_COUNT - i + 4}]/td[1]`, '10');
        }

        // CHANGE: remove also one row **after** the one with number 10
        await testTextContains(driver, `//tbody/tr[6]/td[1]`, '11');
        await clickElementByXPath(driver, `//tbody/tr[6]/td[3]/a/span[1]`);
        await testTextContains(driver, `//tbody/tr[6]/td[1]`, '12');

        await testTextContains(driver, '//tbody/tr[5]/td[1]', '10');
        await testTextContains(driver, '//tbody/tr[4]/td[1]', '4');
    }
    async run(driver: WebDriver) {
        await clickElementByXPath(driver, "//tbody/tr[4]/td[3]/a/span[1]");
        await testTextContains(driver, '//tbody/tr[4]/td[1]', '10');
    }
}

This won't change much the amount of time needed to complete the test, but it will make the test itself more challenging, and realistic, and current React hooks without keys would fail at that.

Last, but not least, it would be surprising if React hooks require mandatory key for every component to grant it works ... and I don't really buy the fact keyed bench have a different paradigm, 'cause every other library simply uses, or not, the key as nice-to-have addiction, not something that would require massive changes around the same logic/test.

[WebReflection]

@krausest looking forward to hear your opinion on this, and please let me know if you'd like a MR for the proposed change, as I've tested already and it works fine, or better ... it breaks fine React hooks without key.

Thanks.

[ivarprudnikov]

I think non keyed is a way more common pattern than keyed, as you don't always have a unique ID to associate to every rendered component [...]

FYI there are couple of things that affect development and usage of keys (in React):

• React docs clearly state that developer should use keys;
• When developing console will output error-like warnings (red ones) if keys are not used;
• When using JSX there is a specific eslint rule for it;

In my practice keys are always present and they will be set to index when elements added through .map() and nothing better exists. Why would developer not use keys if it is a suggested good practice, reinforced with red messages in console? (a rhetoric question - no need to reply)

[localvoid]

I think non keyed is a way more common pattern than keyed

Non-keyed reconciliation is a workaround for vdom-like libraries to deal with static lists. Non-keyed table is just a wall of shame for libraries that unable to correctly preserve internal state.

[thysultan]

@WebReflection Notice that the non-keyed React implementation also uses keys(index as key).

https://github.com/krausest/js-framework-benchmark/blob/master/frameworks/non-keyed/react/src/main.jsx#L147

[WebReflection]

@ryansolid I've rewritten the React hooks test both keyed and non-keyed, using latest proposed lighterhtml pattern, which is also based on the fastest non-keyed library right now, domc, based on a pretty simple and elegant scope solution.

React Keyed

import React, { memo, useReducer, useCallback } from 'react';
import ReactDOM from 'react-dom';

let did = 1;
const buildData = (count) => {
    const adjectives = ["pretty", "large", "big", "small", "tall", "short", "long", "handsome", "plain", "quaint", "clean", "elegant", "easy", "angry", "crazy", "helpful", "mushy", "odd", "unsightly", "adorable", "important", "inexpensive", "cheap", "expensive", "fancy"];
    const colours = ["red", "yellow", "blue", "green", "pink", "brown", "purple", "brown", "white", "black", "orange"];
    const nouns = ["table", "chair", "house", "bbq", "desk", "car", "pony", "cookie", "sandwich", "burger", "pizza", "mouse", "keyboard"];
    const data = [];
    for (let i = 0; i < count; i++) {
        data.push({
            id: did++,
            label: adjectives[_random(adjectives.length)] + " " + colours[_random(colours.length)] + " " + nouns[_random(nouns.length)]
        });
    }
    return data;
};

const _random = max => Math.round(Math.random() * 1000) % max;

const Scope = () => ({
  data: [],
  selected: -1,
  add() {
      this.data = this.data.concat(buildData(1000));
  },
  run() {
      this.data = buildData(1000);
  },
  runLots() {
      this.data = buildData(10000);
  },
  clear() {
      this.data = [];
  },
  update() {
      const {data} = this;
      for (let i = 0, {length} = data; i < length; i += 10) {
        const {id, label} = data[i];
        data[i] = { id, label: label + " !!!" };
      }
  },
  swapRows() {
      const {data} = this;
      if (data.length > 998) {
          const tmp = data[1];
          data[1] = data[998];
          data[998] = tmp;
      }
  },
  delete(id) {
      const {data} = this;
      const idx = data.findIndex(d => d.id === id);
      data.splice(idx, 1);
  },
  select(id) {
      this.selected = id;
  },
});

const listReducer = (state, action) => {
  const {type} = action;
  switch (type) {
    case 'delete':
    case 'select':
      state[type](action.id);
      break;
    default:
      state[type]();
      break;
  }
  return {...state};
};

const GlyphIcon = <span className="glyphicon glyphicon-remove" aria-hidden="true"></span>;

const Row = memo(({ selected, item, dispatch }) => {
  const select = useCallback(() => dispatch({ type: 'select', id: item.id }), []),
    remove = useCallback(() => dispatch({ type: 'delete', id: item.id }), []);

  return (<tr className={selected ? "danger" : ""}>
    <td className="col-md-1">{item.id}</td>
    <td className="col-md-4"><a onClick={select}>{item.label}</a></td>
    <td className="col-md-1"><a onClick={remove}>{GlyphIcon}</a></td>
    <td className="col-md-6"></td>
  </tr>);
});

const Button = ({ id, cb, title }) => (
  <div className="col-sm-6 smallpad">
    <button type="button" className="btn btn-primary btn-block" id={id} onClick={cb}>{title}</button>
  </div>
);

const Jumbotron = memo(({ dispatch }) =>
  <div className="jumbotron">
    <div className="row">
      <div className="col-md-6">
        <h1>React Hooks keyed</h1>
      </div>
      <div className="col-md-6">
        <div className="row">
          <Button id="run" title="Create 1,000 rows" cb={() => dispatch({ type: 'run' })} />
          <Button id="runlots" title="Create 10,000 rows" cb={() => dispatch({ type: 'runLots' })} />
          <Button id="add" title="Append 1,000 rows" cb={() => dispatch({ type: 'add' })} />
          <Button id="update" title="Update every 10th row" cb={() => dispatch({ type: 'update' })} />
          <Button id="clear" title="Clear" cb={() => dispatch({ type: 'clear' })} />
          <Button id="swaprows" title="Swap Rows" cb={() => dispatch({ type: 'swapRows' })} />
        </div>
      </div>
    </div>
  </div>
, () => true);

const Main = () => {
  const [state, dispatch] = useReducer(listReducer, Scope());

  return (<div className="container">
    <Jumbotron dispatch={dispatch} />
    <table className="table table-hover table-striped test-data"><tbody>
      {state.data.map(item => (
        <Row key={item.id} item={item} selected={state.selected === item.id} dispatch={dispatch} />
      ))}
    </tbody></table>
    <span className="preloadicon glyphicon glyphicon-remove" aria-hidden="true"></span>
  </div>);
}

ReactDOM.render(<Main />, document.getElementById('main'));

React non-keyed

import React, { memo, useReducer, useCallback } from 'react';
import ReactDOM from 'react-dom';

let did = 1;
const buildData = (count) => {
    const adjectives = ["pretty", "large", "big", "small", "tall", "short", "long", "handsome", "plain", "quaint", "clean", "elegant", "easy", "angry", "crazy", "helpful", "mushy", "odd", "unsightly", "adorable", "important", "inexpensive", "cheap", "expensive", "fancy"];
    const colours = ["red", "yellow", "blue", "green", "pink", "brown", "purple", "brown", "white", "black", "orange"];
    const nouns = ["table", "chair", "house", "bbq", "desk", "car", "pony", "cookie", "sandwich", "burger", "pizza", "mouse", "keyboard"];
    const data = [];
    for (let i = 0; i < count; i++) {
        data.push({
            id: did++,
            label: adjectives[_random(adjectives.length)] + " " + colours[_random(colours.length)] + " " + nouns[_random(nouns.length)]
        });
    }
    return data;
};

const _random = max => Math.round(Math.random() * 1000) % max;

const Scope = () => ({
  data: [],
  selected: -1,
  add() {
      this.data = this.data.concat(buildData(1000));
  },
  run() {
      this.data = buildData(1000);
  },
  runLots() {
      this.data = buildData(10000);
  },
  clear() {
      this.data = [];
  },
  update() {
      const {data} = this;
      for (let i = 0, {length} = data; i < length; i += 10) {
        const {id, label} = data[i];
        data[i] = { id, label: label + " !!!" };
      }
  },
  swapRows() {
      const {data} = this;
      if (data.length > 998) {
          const tmp = data[1];
          data[1] = data[998];
          data[998] = tmp;
      }
  },
  delete(id) {
      const {data} = this;
      const idx = data.findIndex(d => d.id === id);
      data.splice(idx, 1);
  },
  select(id) {
      this.selected = id;
  },
});

const listReducer = (state, action) => {
  const {type} = action;
  switch (type) {
    case 'delete':
    case 'select':
      state[type](action.id);
      break;
    default:
      state[type]();
      break;
  }
  return {...state};
};

const GlyphIcon = <span className="glyphicon glyphicon-remove" aria-hidden="true"></span>;

const Row = memo(({ selected, item, dispatch }) => {
  const select = useCallback(() => dispatch({ type: 'select', id: item.id }), [item]),
    remove = useCallback(() => dispatch({ type: 'delete', id: item.id }), [item]);

  return (<tr className={selected ? "danger" : ""}>
    <td className="col-md-1">{item.id}</td>
    <td className="col-md-4"><a onClick={select}>{item.label}</a></td>
    <td className="col-md-1"><a onClick={remove}>{GlyphIcon}</a></td>
    <td className="col-md-6"></td>
  </tr>);
});

const Button = ({ id, cb, title }) => (
  <div className="col-sm-6 smallpad">
    <button type="button" className="btn btn-primary btn-block" id={id} onClick={cb}>{title}</button>
  </div>
);

const Jumbotron = memo(({ dispatch }) =>
  <div className="jumbotron">
    <div className="row">
      <div className="col-md-6">
        <h1>React Hooks keyed</h1>
      </div>
      <div className="col-md-6">
        <div className="row">
          <Button id="run" title="Create 1,000 rows" cb={() => dispatch({ type: 'run' })} />
          <Button id="runlots" title="Create 10,000 rows" cb={() => dispatch({ type: 'runLots' })} />
          <Button id="add" title="Append 1,000 rows" cb={() => dispatch({ type: 'add' })} />
          <Button id="update" title="Update every 10th row" cb={() => dispatch({ type: 'update' })} />
          <Button id="clear" title="Clear" cb={() => dispatch({ type: 'clear' })} />
          <Button id="swaprows" title="Swap Rows" cb={() => dispatch({ type: 'swapRows' })} />
        </div>
      </div>
    </div>
  </div>
, () => true);

const Main = () => {
  const [state, dispatch] = useReducer(listReducer, Scope());

  return (<div className="container">
    <Jumbotron dispatch={dispatch} />
    <table className="table table-hover table-striped test-data"><tbody>
      {state.data.map(item => (
        <Row item={item} selected={state.selected === item.id} dispatch={dispatch} />
      ))}
    </tbody></table>
    <span className="preloadicon glyphicon glyphicon-remove" aria-hidden="true"></span>
  </div>);
}

ReactDOM.render(<Main />, document.getElementById('main'));

The neverland non-keyed coutner-benchmark is the following one:

import {
  neverland as $, html, render,
  useCallback,
  useMemo,
  useReducer
} from '../node_modules/neverland-bench/esm/index.js';

let did = 1;
const buildData = (count) => {
    const adjectives = ["pretty", "large", "big", "small", "tall", "short", "long", "handsome", "plain", "quaint", "clean", "elegant", "easy", "angry", "crazy", "helpful", "mushy", "odd", "unsightly", "adorable", "important", "inexpensive", "cheap", "expensive", "fancy"];
    const colours = ["red", "yellow", "blue", "green", "pink", "brown", "purple", "brown", "white", "black", "orange"];
    const nouns = ["table", "chair", "house", "bbq", "desk", "car", "pony", "cookie", "sandwich", "burger", "pizza", "mouse", "keyboard"];
    const data = [];
    for (let i = 0; i < count; i++) {
        data.push({
            id: did++,
            label: adjectives[_random(adjectives.length)] + " " + colours[_random(colours.length)] + " " + nouns[_random(nouns.length)]
        });
    }
    return data;
};

const _random = max => Math.round(Math.random() * 1000) % max;

const Scope = () => ({
  data: [],
  selected: -1,
  add() {
      this.data = this.data.concat(buildData(1000));
  },
  run() {
      this.data = buildData(1000);
  },
  runLots() {
      this.data = buildData(10000);
  },
  clear() {
      this.data = [];
  },
  update() {
      const {data} = this;
      for (let i = 0, {length} = data; i < length; i += 10)
          data[i].label += ' !!!';
  },
  swapRows() {
      const {data} = this;
      if (data.length > 998) {
          const tmp = data[1];
          data[1] = data[998];
          data[998] = tmp;
      }
  },
  delete(id) {
      const {data} = this;
      const idx = data.findIndex(d => d.id === id);
      data.splice(idx, 1);
  },
  select(id) {
      this.selected = id;
  },
});

const listReducer = (state, action) => {
  const {type} = action;
  switch (type) {
    case 'delete':
    case 'select':
      state[type](action.id);
      break;
    default:
      state[type]();
      break;
  }
  return state;
};

const GlyphIcon = () => html`<span class="glyphicon glyphicon-remove" aria-hidden="true"></span>`;

const Row = $(({item, dispatch, selected}) => {
  const select = useCallback(() => dispatch({type: 'select', id: item.id}), [item]),
    remove = useCallback(() => dispatch({type: 'delete', id: item.id}), [item]);

  return html`
    <tr class=${selected ? "danger" : ""}>
      <td class="col-md-1">${item.id}</td>
      <td class="col-md-4"><a onclick=${select}>${item.label}</a></td>
      <td class="col-md-1"><a onclick=${remove}>${GlyphIcon()}</a></td>
      <td class="col-md-6"></td>
    </tr>
  `;
});

const Button = ({id, title, cb}) => html`
  <div class="col-sm-6 smallpad">
    <button type="button" class="btn btn-primary btn-block" id=${id} onclick=${cb}>${title}</button>
  </div>
`;

const Jumbotron = ({dispatch}) => html`
  <div class="jumbotron">
    <div class="row">
      <div class="col-md-6">
        <h1>neverland non-keyed</h1>
      </div>
      <div class="col-md-6">
        <div class="row">
          ${Button({id: 'run', title: 'Create 1,000 rows', cb: () => dispatch({type: 'run'})})}
          ${Button({id: 'runlots', title: 'Create 10,000 rows', cb: () => dispatch({type: 'runLots'})})}
          ${Button({id: 'add', title: 'Append 1,000 rows', cb: () => dispatch({type: 'add'})})}
          ${Button({id: 'update', title: 'Update every 10th row', cb: () => dispatch({type: 'update'})})}
          ${Button({id: 'clear', title: 'Clear', cb: () => dispatch({type: 'clear'})})}
          ${Button({id: 'swaprows', title: 'Swap Rows', cb: () => dispatch({type: 'swapRows'})})}
        </div>
      </div>
    </div>
  </div>
`;

const Main = $(() => {
  const [state, dispatch] = useReducer(listReducer, Scope, {sync: true});
  const jumbotron = useMemo(() => Jumbotron({dispatch}), []);

  return html`
    <div class="container">
      ${jumbotron}
      <table class="table table-hover table-striped test-data">
        <tbody>
        ${state.data.map(item => Row({item, dispatch, selected: item.id === state.selected}))}
        </tbody>
      </table>
      <span class="preloadicon glyphicon glyphicon-remove" aria-hidden="true"></span>
    </div>
  `;
});

render(document.getElementById('main'), Main);

The result is that React Hooks non-keyed benchmark is super fast. Not as fast as lighterhtml, but 1.5X times faster than neverland, which is a bummer, as I need to rewrite the whole v3 branch 'cause I'm everything but satisfied by its results ( basically simply 2X slower than lighterhtml due doubled loops per each template literal hole ... 😭 )

As Summary:

I think this repository should change the removeBench with my previously suggested way, and I think React hook benchmark should land in both keyed and non-keyed folders as reference.

I'm not suggesting to use the provided tests I've used, but those would be already fair.

[ryansolid]

@ivarprudnikov Definitely. Although non-keyed is basically an acknowledgement of index as a key which is React's default behaviour. I was basically saying that index as a key should be avoided at all costs when the intention is to manipulate the list. It's the reason React warns you so heavily for not including it. They also feel that index as a key should be avoided where possible.

@WebReflection The solution doesn't look typical React, but I also don't see any issue with it as long as it doesn't compromise keyed performance in the slightest since that is the category that matters(more below). I think the simple test of removing the row after seems reasonable to identify these offenders if they fail. I haven't tried it yet but that seems like something small that could be implemented.

---

Perhaps this discussion doesn't belong here but I wanted to respond to:

I think non keyed is a way more common pattern than keyed, as you don't always have a unique ID to associate to every rendered component, and while I agree framework incapable of doing non-keyed could be listed under keyed only, and it was the other way around for hyperHTML or lighterhtml implementations, where keyed results were introduced later on as these libraries works more than excellently with non-keyed structures....

I agree that is common scenario for static lists etc, but the idea is to try your best to assign a unique key, whether through hashing or enumeration etc when your list needs to be dynamic. It isn't that most libraries are incapable of doing non-keyed. All virtual dom libraries are non-keyed by default. In fact, all top-down reconciled libraries would be whether they are lit-html, domc, React or Stencil. I'd estimate 90% of the libraries in the Benchmark work this way. They all work more performantly with non-keyed structures. It doesn't make it right though. It's that they chose after seeing the issues with non-keyed to really try to enforce keyed where possible.

Keyed is a concession to the fact that the DOM does in fact keep state and that just moving your data across nodes can have unintentional side-effects. CSS Animations and Transitions are the obvious ones, but I'd argue the most important is Web Components. This is why it baffles me to no end that the libraries that put themselves at the forefront of Web Components didn't come at this stronger with keyed. What if your component has to do expensive computations or async data loading? With a Virtual DOM library that separates the Component from the DOM nodes it occupies there is very little cost hijacking a different DOM node as you are just updating some attributes and your Component state stays intact. But for a Web Component changing that attribute/property could mean a whole new data load or recalculation. Sure you can use caches/global stores but this one example in a whole class of potential issues. Async race conditions, emergent state behaviours (inconsistent data state on transition), layout thrashing. These are all solvable. I just don't understand why avoiding these sorts of issues for your end users wouldn't be a top priority. Even Virtual DOM libraries that don't pay nearly as heavy of a cost still see this as important.

I obviously have a bias as a library writer myself. But I've always been in big support of Web Components and have used them in production for almost 6 years now (thanks for all your polyfills over the years). I'm concerned this mentality of non-keyed being the optimized view is dangerous and misleading. It is an optimization, but one that one should be very conscious of before tripping into especially if Web Components are a consideration.

[WebReflection]

Just to quickly clarify, my two react benchmarks outperform current react hook, and work in non keyed too.

Regarding non keyed bit, my libraries handle everything just fine, without side effects whatsoever, it's only if third parts libraries hold nodes that you might prefer a keyed approach (via reference) so that one ref is bound to a specific node only, but for 99% of the cases, at least in my cases, without vDOM, keyed is overkill, as the domdiff library takes care of updating only what's necessary, instead of trashing nodes in updates.

Regardless, this test suite makes a clear separation between keyed and non-keyed, so I think every framework capable of both, should provide both implementations.

[ryansolid]

Ok awesome. If you've done the work you could probably just make the PR.

Regarding non keyed bit, my libraries handle everything just fine, without side effects whatsoever, it's only if third parts libraries hold nodes that you might prefer a keyed approach (via reference) so that one ref is bound to a specific node only, but for 99% of the cases, at least in my cases, without vDOM, keyed is overkill, as the domdiff library takes care of updating only what's necessary, instead of trashing nodes in updates.

I'm sure it does and most Virtual DOM libraries do too in the basic case. Your libraries are much more similar than they are different. Everything that applies to your library in terms of diffing applies to a VDOM library. And the fact you support keyed is good. Almost Every library supports non-keyed. But keyed actually requires work. If there are missing non-keyed implementations it is because authors/implementors don't want to advertise what they feel is a bad practice.

What I'm saying is that if the DOM nodes that you render contain their own state this can be expensive approach. As you say for your use cases this hasn't been the case, but it definitely trivial to produce such cases against any implementation that doesn't use keyed or referential handling of lists. Simply trigger a slow running CSS transition on a node removal and watch the new data inherit the CSS state. While it could be argued it isn't the responsibility of the library to consider how any outside element might be created, I'd argue that with Web Components where the very nature is the DOM node to contain the state. It's inviting the situation. So even if your library only changes the precise properties and attributes that change, the changing of those attributes can have much bigger side effects than simply moving the DOM node. And worse these changes are something that would be unpredictable by the library. Like if every row has an attribute that you pass a unique data id to, that is used to fetch or process data, maybe render some expensive view. In keyed you'd just move the whole DOM node, but non-keyed even with the minimal changes still will have you changing the attribute and triggering all the internals of each Component in the list whose position has changed.

I mean we can just ignore this case because maybe in reality it has a noticeable detrimental effect in a such a small number of cases, because even when there are bad side effects no one is going to really being paying much attention. But it isn't because of something your library is doing differently. Every VDOM library could make the same call, and they do. I hear this line of rhetoric all the time regardless of the technology approach. I just find 2 things interesting:

1. That most VDOM libraries have taken the key approach now even though non-keyed is more performant for them.
2. Web Components are a potential landmine here, and traditionally VDOM libraries don't typically play nice with them, yet libraries designed to work with them don't seem to prioritize.

Maybe library or approach maturity. I'm not sure. It just doesn't quite add up.

[WebReflection]

if the DOM nodes that you render contain their own state this can be expensive approach

I hear you, and I've implemented keyed approach for neverland too after thinking about it, as I know myself there are (imho rare) cases where this is needed.

Simply trigger a slow running CSS transition on a node removal and watch the new data inherit the CSS state

This is an interesting case, as a node removal takes "zero time" to happen, so it would never have a transition-out visible time, regardless it was keyed or not (it's removed, hence not visible anymore).

I guess my arguing is in regard of real-world use cases. A "transition out of removal" makes sense, as a concept, but in a list of rows it doesn't, as it's been replaced already by something that wouldn't even show a transition-in animation in the keyed case ... which is what makes me wonder about the real value of keyed nodes ... as these just represent a snapshot in the user's visual time, and nothing else, beside the likely expensive work to create such node.

When it comes to non-keyed results, you're updating an existing node, so that all classes, styles, etc, would apply like if the node was new, as in appended, or the same, no visual glitches, and either you want to show the new node got in, and you need a strongly non keyed (fresh new) node in there, or you'll be out of luck with keyed swaps, as the node preserve indeed its previously, already transitioned, state.

these changes are something that would be unpredictable by the library

my libraries handle that with ease, to be honest, so I'm not sure this is a real concern. I've read people in twitter replying to me that "it's impossible to handle non keyed nodes", and so far I guess I've done a great job at doing that, since we target million users, in production, via Custom Elements that are hyperHTML driven, and never faced an issue in this regard.

I mean we can just ignore this case because maybe in reality it has a noticeable detrimental effect in a such a small number of cases

Precisely my conclusion.

most VDOM libraries have taken the key approach now even though non-keyed is more performant for them

it's not about library authors though, it's about use cases ... somebody wrote before "if this is a suggested bad practice, why wouldn't I use it?" ... but on the other hand, "if that's not the right tool for that job, why would you chose the wrong one?"

traditionally VDOM libraries don't typically play nice with them

I can't find real data on this, as VDOM does not really parse node-names or anything, and my Custom Elements based libraries seem to work well with VDOM, so I think this is a common misleading/misconception in regards of Custom Elements.

Of course, if you think about hyped frameworks all based on ShadowDOM, and none of my libraries is based on that on purpose, then I see there might be issues, but I also keep wondering why on earth people still think ShadowDOM was a good idea to start with.

[leeoniya]

This is an interesting case, as a node removal takes "zero time" to happen, so it would never have a transition-out visible time, regardless it was keyed or not (it's removed, hence not visible anymore).

i guess this is worth a mention: https://domvm.github.io/domvm/demos/playground/#lifecycle-hooks

[WebReflection]

@leeoniya those are delayed removals, not the normality, and nothing this repository would ever show, so thanks for the mention, but yeah, we can all tweak any node life-cycle when needed, isn't it?

edit it's an interesting use case regardless, but here I am talking about this benchmark, which doesn't require that, and also, as interesting as it is, I believe newly inserted nodes with animation are more relevant than nodes removed, specially where there's no actual transition between the removed one, and the one that took its place: just an awkward row that doesn't belong to the list anymore while removed (see top 3 bar that makes visually no sense to me).

I also think highlighting removed nodes together with inserted one, is just visual noise, not a real feature for my eyes.

[leeoniya]

I also think highlighting removed nodes together with inserted one, is just visual noise,

the context really matters when making this call. if it's in response to an explicit UI interaction by me, then maybe it's noise. but if it's a reaction to some state change in a status panel not triggered by me, then it can be critical because otherwise the state transition will be instantaneous, unexpected and easily overlooked.

as an example, google analytics does this in their realtime traffic view and it's certainly not just visual noise.

another example where instantaneous rank changes/reordering would be really tough to grok without additional visual cues:

https://domvm.github.io/domvm/demos/playground/#scoreboard

i'm definitely in the camp that hates ui effects for their own sake, but in many cases they provide necessary utility.

[WebReflection]

I don't see any "out-effect" there (just remove Jenny, then Troy, then Mark, and David), only the visual swap, but again, that has nothing to do with this benchmark context (and likely nothing to do with your previous example capabilities).

if there was a benchmark about showing swaps I'd agree keyed is "key" or mandatory, but here we swap row 2 with raw 999, and no frameworks is required to show any special transition so ... I'd like to keep this thread relevant.

[krausest]

@WebReflection: Sorry I'm currently a bit short on time so just a short reply. I was pretty surprised by that report and decided to take a look at the question why doesn't it work without keys? @ryansolid I think there's actually a bug (or at least a major violation of a best practice):

remove = useCallback(() => dispatch({ type: 'REMOVE', id: item.id }), []);

The remove callback doesn't specify its dependencies correctly. I think it should be like that:

remove = useCallback(() => dispatch({ type: 'REMOVE', id: item.id }), [item.id]);

Seems like it works correctly for keyed and non-keyed with that dependency.

Does it have an effect on performance? I don't think so. Here's a comparison of the three versions. WebReflections new implementation (but also with [item.id]), the implementation with the fix above and the old version: Results are close enough not to matter. Does anyone object to using WebReflections's version?

[ryansolid]

@krausest Yeah that makes sense. It's because for keyed you know the id will never change so it's safe. There is no need to ever add the dependency there. For non-keyed you need to add the dependency. It is as simple as that. Probably doesn't make that much of a difference in either case but it is unnecessary for keyed, but is one example where the implementations are conceptually different. The keyed implementation the row is mostly unchangeable except for the label. In the Non-Keyed all fields need to be changeable.

@WebReflection I'm not sure if you've seen https://custom-elements-everywhere.com/. They tested a variety of scenarios and while many VDOM libraries have improved a couple years ago most had issues with events, and attribute and prop binding etc.. I think most other than React are fine now but historically many of these libraries didn't support them well.

[WebReflection]

@krausest thanks for investigating more within the current test. Oddly enough, in my laptop the difference was more relevant, but since @ryansolid said my test is not "the React way", I wouldn't mind updating the current test with your changes that makes sense to me too, 'cause it'd be likely better for others checking the code, and maybe trying the non-keyed version too.

@ryansolid if you look closer, hyperHTML is there and scores 100% from years ago ... so yes, I'm aware of that, and all my libraries would easily deal with that test-case 😉

[WebReflection]

@krausest I still think having the change in the remove bench would help revealing other gotchas:

await testTextContains(driver, `//tbody/tr[6]/td[1]`, '11');
await clickElementByXPath(driver, `//tbody/tr[6]/td[3]/a/span[1]`);
await testTextContains(driver, `//tbody/tr[6]/td[1]`, '12');

it adds not much time to execute that test, but it helps understanding implementations

[krausest]

Just one more thing. I don't consider "non-keyed is faster" as a valid general statement - even this depends on the use case. This benchmark uses alternating row colors and includes rendering duration (it simply looked nicer when I started). This gives non-keyed frameworks an somehow unfair advantage for remove rows. There's a hidden (and not properly working) option to switch to a minimal css without alternating colors for the rows (hint: configureStyles.js). I ran react-hooks with that minimal css and included react with the bootstrap css as a reference. Please don't compare absolute numbers as react-hooks should be faster but I didn't want to take the time to reboot and re-run both or check what in my linux system changed... It's interesting to see that remove row is much faster in the keyed version. The original react version shows that it's the alternating row colors that eliminate this effect.

[WebReflection]

I don't consider "non-keyed is faster" as a valid general statement - even this depends on the use case.

agreed, but check at the swap rows 😱 ... but also at replacing, so that while removal might be not faster, and theoretically the selection should also be faster keyed, the overall personal feeling is that non-keyed are always faster, as a whole.

[krausest]

Yes, but swap rows is carefully optimized to show react's lack of optimization for it :smiley:
For other frameworks it's still slower for keyed but not that much:

[ryansolid]

@ryansolid if you look closer, hyperHTML is there and scores 100% from years ago ... so yes, I'm aware of that, and all my libraries would easily deal with that test-case 😉

Of course your work has been very influential on the Web Component side of things. I was just mentioning that many Virtual DOM libraries weren't as good as they were today. Your libraries have always been at the forefront for Web Components in my mind and I have a lot of respect for your work. Which is why I was so curious about your thoughts on these nagging details of keyed/non-keyed.

[localvoid]

There is no such thing as universal "non-keyed" algorithm, for many vdom libraries it is a different heuristics to improve DX when working with static lists. And libraries that has a similar heuristic to React (majority of vdom libs doesn't) aren't using a simple key={index}, they are using a position relative to all other values in the same array, and values aren't just nodes or components, it can be a nested array or a hole (null/false value).

because maybe in reality it has a noticeable detrimental effect in a such a small number of cases

The main reason why keys are mandatory in React is because when people started using React it was the most common and hard to debug bugs for people coming from different frameworks that used mutable data + implicit tracking by object identity. Mapping data incorrectly to UI state is not a feature to improve performance, it is a bug and that is why all this broken "non-keyed" libraries should be in a separate table.

[WebReflection]

There is no such thing as universal "non-keyed" algorithm

if you mean in the sense of relating unique data with unique nodes, I agree, but there are good algorithms to avoid DOM trashing and promote node recycling, where non vdom libraries I know, or wrote, do a very good job, even with sparse updates (lighterhtml and derivated, but also lit-html).

Yet I often find myself in using keyed approach when it's possible, but it's not always possible, so falling back to non-keyed in a way that works, seems like a feature to me.

The main reason why keys are mandatory in React ...

I don't think that's true, as my non-keyed benchmark file (posted before) works just fine without keys. Maybe React does the keying automatically for me behind the scene, but it seems to be both faster than keyed, with a deviation from lighterhtml arund 0.15+, instead of 0.55+, and useful for some use case, like a well known, never holed (and btw, null holes are a bug, imho), lists of data that might not contain any unique id.

Mapping data incorrectly to UI state is not a feature to improve performance, it is a bug and that is why all this broken "non-keyed" libraries should be in a separate table.

I agree non-keyed should have a table a part, but I wouldn't call libraries capable of non-keyed views broken. Anyway, it looks like this thread is shifting to other topics, but overall seems resolved to me .. should I close this?

[localvoid]

If you try to build your react example in a dev mode, you'll notice that it prints warnings in console that you should add keys. Heuristics for non-keyed rendering in vdom-like libraries aren't designed for use cases like this. If you remove first item in a list of N items, you'll need to perform N-1 updates. You won't be able to treat components that you render in this list as black boxes to make sure that they don't have any transient state (something like waves in material design buttons, uncontrolled inputs, etc), so it isn't suitable for large scale app development. Maybe it makes sense to apply this strategy in this benchmark (because it has a small set of test cases) to get better overall score if you are interested in benchmarketing, but it is a really bad general purpose strategy for handling such use cases.

To avoid DOM trashing, there are completely different strategies and some UI libraries support recycling (components/DOM nodes), and they can correctly map data to existing UI state with this strategy. To handle majority of edge cases it is even possible to introduce reset lifecycle to components and always reinsert DOM nodes to make sure that display tree state is resetted. React doesn't support such strategy. It is all about tradeoffs, some developers don't think that it is worth it, as it doesn't improve overall time by a lot, unless you are trying to use a synthetic benchmark that doesn't flush DOM changes and inflates benchmark numbers.

Also, there are strategies like Vue keep-alive, but to handle all edge cases it requires additional component state active and component developers will need to handle this state transitions. In my opinion it is also not worth it, because it adds even more complexity for component developers.

and btw, null holes are a bug, imho

When you render a static form, it is often necessary to conditionally render some elements, holes would allow you to map data correctly to UI state without using any keys.

[WebReflection]

As the test case has been updated, I can now close this issue. Worth mentioning that the test, as it is, might work even for non-keyed now, if anyone would like to file a PR with that 👋