[DevTools] Hotfix: Fixed element tree sorting when we have an array of entries.

[hristo-kanchev]

Fixes: #16843

Description:
The bug was caused due to the use of Object.entries in the InspectedElementTree component.

We aren't taking into account that the data prop could change and that the data entries could be an array.

If we removed the second entry out of three entries in total the index will shift and thus remove the last one which is the incorrect one.

@bvaughn Could you check this out? Thanks!

[sizebot]

No significant bundle size changes to report.

Generated by 🚫 dangerJS against ebfab31

[bvaughn]

I don't think this would fix the issue. (Did you try it? Does it actually change behavior?)

I think the underlying issue is that the key (an array index in this case) gets a new value, but the UI doesn't update to reflect that because of how we're managing editable values in state. I think the "fix" to this would require us making useEditableValue a bit more complicated- to not mask changes unless the user actually updated the value, but this is also a bit tricky since you wouldn't want to replace out from under a pending edit.

I was thinking about this during my coffee walk this morning. I might take a pass at it (unless you really wanted to tackle it?)

[bvaughn]

Thanks for jumping on this with a PR so quickly by the way :)

[bvaughn]

The fix for this is tricky, because the intuitive one would essentially boil down to this anti-pattern:
https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#anti-pattern-erasing-state-when-props-change

Need to reset the state in the case where (a) external prop/value changes (b) no local edit has been applied to mask and (c) there isn't a pending edit (maybe even the input isn't focused).

[hristo-kanchev]

I tried it out and it actually fixed it.

I can check it out in further detail early tomorrow morning, if you'd like?

I think initially when I submitted the PR for the prop/state editing interface I didn't see this issue because of https://github.com/facebook/react/pull/16700/files/389010a27e9a0bb0f332dff844ff744b23e1a5e0#diff-3230479a214c02a97506fb71c4315109R41

As you said this uses https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#anti-pattern-erasing-state-when-props-change which is 😒

And, no worries, it's been an absolute pleasure to work on DevTools!

[bvaughn]

Interesting. I tested it out and it didn't fix it. I wonder if we're using slightly different test cases? :)

We should probably add some unit tests for this too! So we don't regress.

[hristo-kanchev]

That's weird. I tried building the extension with yarn test:firefox in the extensions package and and it was fine.

I agree, we definitely need to expand our tests in order to not regress 👍

Edit: I used the test cases described in the issue.

[bvaughn]

This repros the issue for me:

function ArrayOfStrings() {
  const [items, setItems] = useState(["a", "b", "c"]);
  const addItem = () =>
    setItems(prevItems => {
      const charCode =
        prevItems.length > 0
          ? prevItems[prevItems.length - 1].charCodeAt(0) + 1
          : 97;
      const newChar = String.fromCharCode(charCode);
      return [...prevItems, newChar];
    });
  const removeItem = () =>
    setItems(prevItems => {
      return prevItems.slice(1);
    });

  return (
    <div>
      Items array ({items.join(",")})<button onClick={addItem}>Add item</button>
      <button onClick={removeItem}>Remove item</button>
    </div>
  );
}

Removing items from the start of the array should cause all of the strings to update and shift forward, but they don't change.

[hristo-kanchev]

Ahh, yeah, now I see the problem (thanks to your repro code). This PR won't fix the issue.

You are correct, the useEditableValue is the culprit.

[hristo-kanchev]

Closing PR for now, as this won't actually fix the issue.

[LetItRock]

Hi!

If I can put my two cents into it...

I wasn't able to reproduce the issue with the example code from @bvaughn where the state is changed via hooks... but with the code example from author of the issue, where the state changes in class component...

I see that when InspectedElementTree is created with state data it delegates rendering values of each prop to KeyValue component. That component for simple editable values (number, string ...) renders EditableValue, but when the value is an array - for each item it renders again (recursively) KeyValue component.
The problem that I see here is that KeyValue component has a key property which is an index of the item from the array. As you know it's not safe to use index when the order of items in the array may change.
I've quickly fixed that by simply concatenating index and value for the case when value is string/number... and it resolves the issue here...

---

Before fix:

1. 4 items selected - OLT_USD, MRL_USD, HQH_USD, RHD_USD
2. HQH_USD unselected - then state updated to - OLT_USD, MRL_USD, RHD_USD
3. DevTools shows - OLT_USD, MRL_USD, HQH_USD - because of index issue

---

After fix:

---

@bvaughn, @hristo-kanchev wdyt guys?

[bvaughn]

As you know it's not safe to use index when the order of items in the array may change.

This is true in most cases, but in this particular case- I don't think there's a better available key.

Using a key that includes the value would improve some use cases, but it also has a few downsides- it would mean throwing away and recreating a lot of elements when the value is edited, and it wouldn't work for arrays that contained elements with non-unique string values (e.g. [object Object]).

[LetItRock]

Using a key that includes the value would improve some use cases, but it also has a few downsides- it would mean throwing away and recreating a lot of elements when the value is edited, and it wouldn't work for arrays that contained elements with non-unique string values (e.g. [object Object]).

@bvaughn I meant to use only index+value for arrays with simple values like strings, numbers. For arrays with object we could use index as how it is right now.
When developer will edit simple value from the array, then that only one edited item would be thrown away. If an array contains objects - only props of those objects could be edited, meaning that components displaying object properties would be reused.
Correct me if I'm wrong here.

---

I would love to understand your idea regarding useEditableValue better if you can explain it more to me.