d4 icon indicating copy to clipboard operation
d4 copied to clipboard

missing `key` in the demonstration code

Open Pomax opened this issue 7 years ago • 2 comments

The demonstration code of https://d4.js.org shows the following example code:

const paths = voronoi(samples)
    .polygons()
    .map(sample => (
      <path
        d={`M${sample.join('L')}Z`}
        fill={color(sample.data)}
        stroke={color(sample.data)}
      />
    ));

but this is missing a key for React to do efficient DOM diffing. Without it you'll get both console warnings, and potentially slow operations where two samples being swapped causes React to instead rebuild them instead of just swapping them.

For properly taking advantage of React, you'll want something like this:

const paths = voronoi(samples)
    .polygons()
    .map(sample => (
      <path
        key={sample.id}
        d={`M${sample.join('L')}Z`}
        fill={color(sample.data)}
        stroke={color(sample.data)}
      />
    ));

where sample.id is a unique identifier for a sample.

And note that, while tempting, you can't use map( (sample,idx) => ....key={idx} ) because the sample's position in the list is not uniquely identifying for the sample (that's only unique information for the specific list the samples are currently in), so successive calls with a reordered array would still make React perform far worse than if a proper unique value is used as key.

Pomax avatar Jul 18 '16 19:07 Pomax

For the particular example you chose isn't the # of elements predetermined? You should never get additions and replacements happening if you were to animate. For instance I used that code with my own virtual dom tool and found it reused all the inner nodes since they matched the previous. Does React not behave the same way?

Here's my code for comparison:

const width = 960;
const height = 500;
const voronoi = d3.voronoi().extent([[-1, -1], [width + 1, height + 1]]);

const color = (d) => {
  const dx = d[0] - width / 2;
  const dy = d[1] - height / 2;

  return d3.lab(100 - (dx * dx + dy * dy) / 5000, dx / 10, dy / 10);
};

const Mesh = () => {
  const sampler = poissonDiscSampler(width, height, 40);
  const samples = [];
  let sample; while (sample = sampler()) samples.push(sample);

  const paths = voronoi(samples)
    .polygons()
    .map((sample, i) => {
      const fill = color(sample.data).toString();

      return diff.html`<path
        d="M${sample.join('L')}Z"
        fill="${fill}"
        stroke="${fill}"
      />`
    });

  return diff.html`<svg width=${width} height=${height}>${paths}</svg>`;
};

const render = () => {
  diff.innerHTML(document.body.querySelector('main'), Mesh());
  requestAnimationFrame(render);
};

render();

tbranyen avatar Jul 19 '16 20:07 tbranyen

In your exact example it's not a true problem, but people take example code and then tweak it to get their own version, so if someone takes your example but throws in one or more calls that reorder the content (say a toggled .reverse() between the .polygon() and .map() functions) then their code may suddenly behave quite poorly compared to what it could be doing.

Generally it's a good idea to have key attributes even if you're sure the order never changes, because current you can never know for sure whether future you won't change their mind =)

Pomax avatar Jul 20 '16 17:07 Pomax