Preact: Preact 8.0.1 - Rendering Lists with duplicate key values causes "ghost" elements

[rubencodes]

I recently upgraded to 8.0.1 and noticed an issue. I was having a bug where I was accidentally rendering a list with duplicate key values, and I noticed elements seem to leave behind "ghosts" on re-render if their key values are duplicated. These ghosts are visible both in the DOM and in React DevTools. A simple reproducible example of this is:

class App extends Component {
    render() {
            return (
                <div class="App">
                    { [1, 2, 3].map(() => (<span key={ 0 }>Hello</span>)) }
                    <button onClick={ () => this.forceUpdate() }>Rerender</button>
                </div>
            );
    }
}

EDIT: See on CodePen.io here.

Every time this component "rerenders", it will grow in size, leaving behind ghosts.

[leeoniya]

since keys' sole purpose is to uniquely identify similar sibling objects, is undefined behavior in the presence of duplicate keys really that surprising?

what were you expecting to happen?

[rubencodes]

I'm aware it's not the way keys should be used, and that perhaps this behavior is a-okay since they're not supposed to be used that way, just seemed like an unusual thing to happen and something I wasn't sure you all were aware of. I believe [email protected] handled this edge case more gracefully (can't say for certain what the behavior was then). If this behavior is acceptable, I will happily close the issue.

[NekR]

If it was working correctly in Preact 7 and doesn't in Preact 8, then it potentially may cause some problem in other situation. i.e. unexpected code/behavior is in place now.

Even if this isn't a regression or it's fine as is, it at least seems like a good case for preact/debug, @developit don't you think?

[developit]

Absolutely something preact/debug would be amazing for - let's get that added for sure!

FWIW, I don't think duplicate keys has ever really worked properly in Preact. The keys get used to build a map, so dupes just overwrite eachother. Perhaps before 8.x the keys were being ignored, since that was that case for Pure Functional Components?

[iam4x]

I'm also experiencing this issue with preact ^7 and ^8 when I have a component (class or functionnal) with a key={ 0 } or key={ '0' }

My actual workaround fix is to key={ n + .1 } which does the job.

[NekR]

@iam4x keys are always converted to strings. Why do you need keys there in a first place?

[developit]

Hmm - I thought we used == to check keys? key="0" and key={0} should be the same.

[NekR]

@developit afaik you rely on Object keys. e.g. set keys[key] = node and check keys[key] != void 0. Type of check isn't related here since Object keys are always strings (except of Symbols but who cares). So it's strings.

[iam4x]

@NekR Had this issue with the usage of https://github.com/airbnb/rheostat

You can display "pit points" at every X values which are numbers, when you display a pit point at the value 0, on the next update/render it won't override the existing DOM element for this pit point but will re-create a new one (like the initial description of the issue).

Exact line of where it happens is https://github.com/airbnb/rheostat/blob/master/src/Slider.jsx#L662

If you want, I can make an easy repro case.

[developit]

repro would be great!

[rubencodes]

fwiw I added a CodePen.io to reproduce my issue above :)

[developit]

I'm a little stuck here because I can't think what the correct behavior would be here. Should it just try to ignore duplicate keys entirely?

[thysultan]

In terms of correctness react might not render what you want if you expect it to render 3 items https://codesandbox.io/s/V6rEgX9, I think ivi handles this the correct way http://codepen.io/thysultan/pen/OmyqgJ?editors=1010

[iam4x]

Repro case with rheostat: https://github.com/iam4x/preact-key0-repro-bug

[developit]

It seems like duplicate keys should produce a single element.

[robertknight]

The issue appears to be resolved in Preact 10: https://codesandbox.io/s/1q15no18k7. Keys are no longer strings in Preact 10 AFAIK, so the '0' vs 0 conflict should be resolved (can you confirm @developit?)

An outstanding question though is to decide whether we want to guarantee this behaviour. I would be inclined to say _no_, on the basis that it could complicate internal changes in future, unless there is a significant React ecosystem compatibility reason. I suggest the outcome here needs to be of three things:

1. We say the behaviour is undefined, close this issue and do nothing
2. We say the behaviour is undefined, that debug mode should warn about it and create a follow-up task for that
3. We say that Preact 10's current behaviour is supported and add a test case

Thoughts?

[marvinhagemeister]

I'm happy to inform that we went with 2). We do warn in preact/debug when we encounter a child with a duplicate key. It's still treated as undefined behaviour, but now that there is a warning it's hard to miss :+1: