React: Bug: Component with a Symbol as key, causes Crash

[omarsy]

React version:
16.13.1

Steps To Reproduce

1. Go To the link https://codesandbox.io/s/happy-ramanujan-xlegp?file=/src/App.js
2. We can see errors caused by this part of code: https://github.com/facebook/react/blob/6fddca27e75950adda92ab4f4946442907dc3bb7/packages/react/src/ReactElement.js#L228

Link to code example:
https://codesandbox.io/s/happy-ramanujan-xlegp?file=/src/App.js

The current behavior

Crash

The expected behavior

No crash

[bpernick]

So I guess the 3 options I see would be:

1. Disallow Symbol keys but handle them with a graceful error message that doesn't crash the app
2. Allow Symbol keys but convert them to a string with Symbol.toString()
3. (possibly) Allow Symbol keys to remain as symbols

I think I'd lean toward option 2 -- what do y'all think?

[kunukn]

A possible solution for option 2?

if (hasValidKey(config)) {
  key = typeof config.key === 'symbol' 
    ? config.key.toString()
    : '' + config.key;
}

[bpernick]

@kunukn you read my mind. Working on that pr now :)

[bvaughn]

FWIW, keys should be strings. (So far as I'm aware, this has been the case since 2014, 5aab0bddaa9dd2684049194a2488d57517d89cae.) The '' + key fallback was a convenience concession for Array indices-as-keys.

Some previous discussion about this: #11996

This comment in particular raises a good concern wrt using symbols for keys: https://github.com/facebook/react/issues/11996#issuecomment-411831270

Although I suppose you could argue that using Symbol.for would negate that concern.

[bpernick]

Hmm thanks Brian! It looks like fully supporting symbol keys is unlikely to be useful and likely to be dangerous because it could circumvent React's checks for duplicate keys among siblings.

Rather than actually supporting Symbol keys I just tried to make sure they were automatically stringified. The 3 advantages I see are:

1. Now Symbol keys are treated the same way as number keys, etc.
2. Symbol keys don't automatically crash the app.
3. If the dev does something bad, like " return (<>
   ", they will get a descriptive error about duplicate keys, rather than the arcane one Omar received.

Let me know what you think -- If we decide no, perhaps another solution would be to disallow symbols explicitly in hasValidKey

[bvaughn]

My initial inclination would be to add a DEV-only check + warning that symbols-as-keys isn't supported.

That being said, I don't really speak for the team on this particular issue :smile:

[bpernick]

@bvaughn Ok! Happy to do that if that's what we decide :)

[juanjorge]

Hmm thanks Brian! It looks like fully supporting symbol keys is unlikely to be useful and likely to be dangerous because it could circumvent React's checks for duplicate keys among siblings.

Rather than actually supporting Symbol keys I just tried to make sure they were automatically stringified. The 3 advantages I see are:

1. Now Symbol keys are treated the same way as number keys, etc.
2. Symbol keys don't automatically crash the app.
3. If the dev does something bad, like " return (<>
   ", they will get a descriptive error about duplicate keys, rather than the arcane one Omar received.

Let me know what you think -- If we decide no, perhaps another solution would be to disallow symbols explicitly in hasValidKey

The third point is not true, you won't receive an error, on that case you will receive 2 different values.

[bvaughn]

If the dev does something bad, like " return (<>

", they will get a descriptive error about duplicate keys, rather than the arcane one Omar received.

The third point is not true, you won't receive an error, on that case you will receive 2 different values.

The context of the above comment is that if React called toString on symbols, then two symbols which were not equal before (Symbol('foo') !== Symbol('foo')) would now be equal ("Symbol(foo)" === "Symbol(foo)").

That is, unless you're suggesting we allow Symbol as key without converting to a string– in which case, we'd need to consider how symbols would work with server side rendering (how they get serialized and sent to the client). For example, even if React handled the serialization– how would it know how to reconstitute a symbol on the client? Should it use Symbol("...") or Symbol.for("...")? Each one is significantly different in terms of behavior.

[juanjorge]

yeah, that's a good point, the hydrate, in the global context, should get it using Symbol.for("...") in the client, but should be tested, im not sure..

[bvaughn]

I agree that using Symbol.for("...") would probably be the better option, but there's the possibility that Symbol("...") was used intentionally (to create unique symbols regardless of their string content) at which point– using Symbol.for("...") during hydration would almost certainly cause errors.

I don't know if this was part of the reason why symbols have never been supported as keys 😄 I don't think I've ever had a conversation about this with other team members. Does seem like an added consideration though.

[omarsy]

My initial inclination would be to add a DEV-only check + warning that symbols-as-keys isn't supported.

That being said, I don't really speak for the team on this particular issue 😄

I think we have to deal with symbols as we deal with objects. At the moment when we add an object as a key we don't get an error message or a warning we get the key which becomes [object Object].
If we add a warning for the Symbol we have to do it for all the other types that are not a String.

[omarsy]

But I think that if javascript supports Symbol as the key to an object we should also support it. But maybe I'm wrong

[bpernick]

Any further thoughts on this?

[johnloven]

At the moment when we add an object as a key we don't get an error message or a warning we get the key which becomes [object Object].

As a React developer for the last couple of years I did not know this was the case. And the docs doesn't really make that clear either. I think it would be helpful to show a dev warning for all non-primitive keys, as well as make it clear in the docs that all keys are converted to strings.

[bpernick]

So in addition to the things below, I tried adding a dev warning for any type of key other than a string or number. What do yall think?

1. Now Symbol keys are treated the same way as number keys, etc.
2. Symbol keys don't automatically crash the app.
3. If the dev does something bad, like " return (<>
   ", they will get a descriptive error about duplicate keys, rather than the arcane one Omar received.

[bvaughn]

Someone will have to review the PR, @bpernick. This often takes a while as there are only a couple of core contributors and we're juggling a lot of things. 🙇 Sorry.