React Devtools should produce a better error message when integers are present as keys on react elements

[alexkolson]

Current behavior: React Devtools throws "RangError: Invalid Array Length" when integers are used as keys on react elements.

Example:

https://codesandbox.io/s/interesting-violet-v5c5j

https://v5c5j.csb.app/

Using anything but strings as keys is as far as I understand not even correct usage, but it would be great if react devtools checked a little bit earlier and had a nicer error than "RangeError: Invalid Array Length." It takes a long time to figure out from this message that one somehow managed to use integers as keys and needs to correct it.

I've only tested with Chrome and the latest version of react devtools as a chrome extension.

[donaldnevermore]

According to the example, it seems like if you don't explictly provide a key like this

<ListItem key={0} />

it would escape from hasValidKey here in

// react/packages/react/src/ReactElement.js
export function createElement(type, config, children) {
  ...
  if (hasValidKey(config)) {
    key = '' + config.key;
  }
}

So the key was never converted to a string.

And because of

Object.assign({}, e, { key: idx })

it also escaped from validateExplicitKey here in

// react/packages/react/src/ReactElementValidator.js
function validateExplicitKey(element, parentType) {
  if (!element._store || element._store.validated || element.key != null) {
    return;
  }
  ...
}

Finally, a NaN appeared somewhere(needs more info) like this.

// RangError: Invalid Array Length
new Array(NaN)

Is it OK to validate if key is a string in validateExplicitKey?
I need some suggestions.

[alexkolson]

Im just a React user but I would prefer if React itself didnt try and prevent deveopers like me from doing unintentional silly things but rather if React-Devtools helped point out my mistake better. The example is a bit contrived but is something I encountered in a production codebase and it took forever to figure out that we were generating our keys very wrong and that that was the cause of the RangeError. Also a side effect is that as soon as Devtools encounters this error it stops working for that page (obviously) and you can't use any of the cool functionality devtools offers.

So while you're trying to debug why the RangeError is happening you can't actually even use the devtools which makes the problem even harder to trace. I hope that makes sense somewhat.

[gaearon]

1. It is definitely supported to pass numbers as keys
2. DevTools shouldn't ever crash no matter whether there are mistakes in the app or not

So this is probably a bug we need to fix. I suppose we assume here that it's gonna be a string here because createElement gives us that guarantee. But by Object.assign-ing it and copying it, you circumvented that guarantee. Even types inside React itself assume fiber.key is a string so this seems like something we'll have to think about more.

[donaldnevermore]

fiber.key is assumed to be a string here but it's not.

str.length could be undefined and pendingStringTableLength could be NaN.

The crash happens here.

Preventing DevTools from crashing by catching this kind of error would be great.

[bvaughn]

The way the keys are being assigned in this example is pretty unusual:

export const genKeys = elements => {
  return elements.map((e, idx) => Object.assign({}, e, { key: idx }));
};

ReactElement automatically stringifies non-string values:

react/packages/react/src/ReactElement.js

Lines 182 to 194 in 5faf377

	// Currently, key can be spread in as a prop. This causes a potential
	// issue if key is also explicitly declared (ie. <div {...props} key="Hi" />
	// or <div key="Hi" {...props} /> ). We want to deprecate key spread,
	// but as an intermediary step, we will use jsxDEV for everything except
	// <div {...props} key="Hi" />, because we aren't currently able to tell if
	// key is explicitly declared to be undefined or not.
	if (maybeKey !== undefined) {
	key = '' + maybeKey;
	}
	if (hasValidKey(config)) {
	key = '' + config.key;
	}

But this example is modifying the element (really, copying it- since elements are frozen/read-only) to assign a numeric key.

DevTools could support this by making our key check more robust, but it technically violates the internal Fiber type expectations so other things could also break:

react/packages/react-reconciler/src/ReactFiber.js

Lines 139 to 140 in 5faf377

	// Unique identifier of this child.
	key: null | string,

Generally, modifying React elements after they are created seems sketchy. I'm not sure if this is something we should both with supporting.