Incorrect DOM result with certain 'key' and ContextProvider combination

[turjmner8]

I confirmed this seems to be an issue in the latest 10.29.0 release, though I haven't tested with 11 beta.

Describe the bug
When combining specific preact 'key' and ContextProvider usage, I am seeing unexpected DOM results compared to the Vnodes generated.

To Reproduce

Go to the codepen below, and follow the instructions listed. - Basically expand all rows, and then collapse either row 1 or 2 and see the order of rows incorrect compared to the expected DOM.

https://codepen.io/turjmner8/pen/XJjVJyZ

I've tried the same type of setup in react as well, but do not see the same issue:

https://reactplayground.vercel.app/#N4IgLgziBcBmCGAbCBTANCAbrK1QEsA7AExQA8A6AK1xHwFsAHAewCcwACAQUcY9lbN6HAOQUA9D0bUIZEQB1CDFuw4AlFPADGnAUNGtNOhUqZtOwDlsPwwKNc2acAvv0HCRNnQFpiQ8VqI+CiEYCaKWsyEEJyRofBEKKwcALwcfloArvQhYBQA5ihgAKKIKDmhAEIAngCSxAAUno5hAJQRUTEcgk6pVjZ2Dk4NcWAJhEnthD15hiRJDYoc6kZ51pp2peW5i4TLyxraeQDKYKz4OgCyzKRoS-uEmYiId3v7hzoU67YoWxVgDSkU2WU1aIAwUhkZBgdDMqg+YDQHEsAGEhCwJqEkd87GjQuQXG59J5ViIANyKZTmZH9Db2FocVx6DxeMC+fyBYKhcmKDrRTjFMiMeAkeAAIzKeLsZE4aRxKClBIaj2erQphD5XTATiQABUbswIH0ANr3SzSsDQUQARhE2PRZTsxCtZ0yKEZr2W5oJVpE1q4dqsDqKKGd-CQqA9Zo4Ft91sqgciTEdoatCGQ7ucnppsZtKMTwadaYjmez3plvoATAXkyGw67S9HcyJKwH7bWi+GM1G3uXLaJKwn24wU2H05Gs02fQP88PR8Xu5PezHpyIAMw1kd1l2sN09r0riuiNdtoMd1Ndidlw-99dDs9bzvjxvL5tr2cP+eXzOKAC66oiRB4AgI0ABlxRQRAOAJEJiCNNEzExCx7jmUhWAaVpkXuZY4i6c0DSRIhSDIJFDHoZhMBQfU-EZPowAAC3wCAKEYQRGAgdV9m6IpMlYPZdi45YAB4gg4ABrFBqhSYBtT8CgLWcAA+bDBJkg15IJZxgBEURnBUrjgAaWTmA0mVUhSNI-R0gAfayY3Ui1zMs6sOFs+y5McizLI3TCADJfI4ATBK4oSxUyMBtT2MBqkYFAUnkEAwoiqIEo4KIUSCLQxOkjDUkU7jyMo6jmCMgiOCI8hWiU-Tgo4QAZchqkLxCSyLlLeYKqpqoTxCCNquLVe49MIIbAOAo1itApjOBgkh4PRKJciwt4Yh+PpgBU4yICtU12v2Ps403L9n33VSbyrQ7t2-E6DLO48LqfEtGRU39s2WCYAHdajsehfRMZZnAAt4yIoqiDT6Uq-EI+YyEwlJ8vW3bcM4TaTQoNGGKYigVrseSDQgf9eV2-BYEC4zTNlCybRETCEeC4nSfUpNH1DGnGvcw0sZHC4UAaCqSI4a0kX2qm50u47nAG3auM2znMp5vmOAAagFpE10l4LXEgyNadqmWIC5rR5ehpFBfssZEGKiBjQABhes29Tx41rTt7Vzct41KxdnULcdtdf3VwShq4zXu3piGTM8ymW2ppa6ZJ8OvkLFnY91vHZe53njZVnNV2rUWHsXAPgr1g2jeIpXs7VzjA+g7sdeLtP9blzPiJNpFXYdvwrYAFi9t3HYAVj7zvDWNAA2Yefa740AHZ-er4OVJD7WarDsmmdHVmpf2Evm75tuc6Pdd7ovcWi8E3eM4V5XTarmrl-deuL8b0uW-IA+O6n0eAA5J-dgBOP+jtrS23bt7d21pnbn3+kvQm0tGLMVQGAU4PwGj4S7oyAOAM4HcXmOhLeXEkY5jxrRNIGNEFjDsIDLihgwC8X4l1J4fVapqS7hQeg8BGANHDlDYisNmG1XZuTJyVNXJ2TJpHZyNlxEOQJCI4+HAAD8gU2YhUFMKUUEoFRRAtBQAACoITA+A0LiUktJCRmkOCYCQG6aSKpEDVW3oIoS4ExSQXZuYg0rg+bST5q4YGRUDTmIQRQAJoM-CuHEAIwRHBurqJFMQcUkodEEn0YY4xSRokdQ4FaIKMTYmuPcRJKSrCI6WOMp4iJ5Voa+Ohv48oINirBMxmE4qkSsmCWBLVTq29upMJUlgxQI1CBEKkODfhsSJpTQ4FEwmhNyAqE4KQBATxOBSHBCAGI1QyhfBAjCCgYowB7B1rAHR3gID4AAF4oCtJWMi6pnAgGcM4IAA

Expected behavior

The expectation is that the sub 'a', 'b', and 'c' rows should always either be shown or hidden below the parent row. The JSX seems to produce this, and using preact dev tools seems to reflect that order correctly in the Vnodes, but the actual realized DOM does not.

If you remove the 'key' setting from the 'li' elements, things work as expected. If you instead do not specify the ExpandableContext.Provider at all, OR specify it around all Label elements, things also work as expected. It seems to be that the key usage in addition to the conditional ContextProvider usage triggers this strange behavior.

[rschristian]

Usage error I'm afraid, any time you're mapping over content you really should be setting keys. Add key props to the provider & label element here and the issue will disappear.

 {todos.map((todo, index) => (
     // IF YOU REMOVE THE CONTEXT PROVIDER, OR PROVIDE IT FOR ALL ITEMS, NO ISSUE IS SEEN
 	todo.text === "1" || todo.text === '2' || todo.text ==='3'
-       ? <ExpandableContext.Provider value={null}>
+       ? <ExpandableContext.Provider value={null} key={todo.text}>
 	      <Label todo={todo} index={index} removeTodo={this.removeTodo} />
         </ExpandableContext.Provider>
- 	  : <Label todo={todo} index={index} removeTodo={this.removeTodo} />
+ 	  : <Label todo={todo} index={index} removeTodo={this.removeTodo} key={todo.text} />
 ))}

Without keys, you're leaving it up to the framework to guess how this is meant to be handled.

We have a tutorial page covering this & more: Keys - Preact Tutorial

[turjmner8]

Yes, I realize I should be setting keys differently, but why would setting a key somewhere downstream break the resulting DOM? Its not like there is any state being carried over by reusing Vnodes inefficiently? The resulting VNode order looks correct, but the actual DOM output does not match.

[marvinhagemeister]

That's an interesting usage of keys not in the way they are supposed to be used. Keys need to be specified on the direct JSX child of anything inside .map(...) calls or other constructs where you have a growing list. Putting them further away defeats their purpose.

// WRONG, key is not on direct jsx child
<ul>
  {list.map(item => {
    return <SomeComponent>
      <li key={item.id}>{item.name}</li>
    </SomeComponent>
  })}
</ul>

// CORRECT, key is on direct JSX child
<ul>
  {list.map(item => {
    return <SomeComponent key={item.id}>
      <li>{item.name}</li>
    </SomeComponent>
  })}
</ul>

// CORRECT, key is on direct JSX child
<ul>
  {list.map(item => {
    return <li key={item.id}>{item.name}</li>
  })}
</ul>

The way it is set up in the shared snipped is effectively an unkeyed diff. That said there is certainly something fishy going on here.

[turjmner8]

My general understanding was if we're not setting keys on the top level, that means we're implicitly opting in to having an index-based key at that level. That could lead to efficiency issues in general, and also functional issues if the downstream children have state that gets carried over, but neither of those were a real concern in our product. This is a silly example (based on the preact todo list playground) to simplify the case I was seeing this in, but even setting a key equal to the index on the top level ContextProvider or Label element being returned by the map seems to resolve the issue, making me think there is a real bug here where the lack of setting any key at that level is behaving differently than explicitly opting into an index key behavior.

That's why I thought this was worth filing - in either case I don't believe the resulting DOM would be appropriate.

[JoviDeCroock]

We are a bit more sensitive to this but index-based keys, just like react, are useless because they never change i.e. a stable-id will prove that item 1 in the index moves to slot 7 while an index based approach to keying will always just mean something moves or shifts...