Component setState doesn't allow Symbol keys

[gingur]

https://jsfiddle.net/gingur/eqprn74c/

When trying to do something like this.setState({ [someSymbol]: 'someValue' }); the render is being invoked properly but the this.state[someSymbol] is undefined. Running Object.getOwnPropertySymbols on this.state I have confirmed that this.setState does not define Symbol properties.

[gaearon]

What is the use case for using Symbols as state keys? State is already private and is never shared so it’s not clear what the benefit of supporting Symbols as keys is. Would you mind sharing your thoughts on this?

[gingur]

Sure, check out https://jsfiddle.net/gingur/4ury0beq/2/. The idea is a Symbol can be provided as a prop to map a "dynamic" state property. Now given, this could also be accomplished with unique strings but the use case at least is demonstrated.

[zpao]

cc @sebmarkbage

Basically this boils down to the fact that the Object.assign "polyfill" we have inside React doesn't handle symbols. We just use that to assign the pending updates into the current state -

react/src/renderers/shared/reconciler/ReactCompositeComponent.js

Lines 762 to 771 in 22a3d03

	var nextState = assign({}, replace ? queue[0] : inst.state);
	for (var i = replace ? 1 : 0; i < queue.length; i++) {
	var partial = queue[i];
	assign(
	nextState,
	typeof partial === 'function' ?
	partial.call(inst, nextState, props, context) :
	partial
	);
	}

. We knew that going in when we first added it but that was a long time ago and symbols are a real thing that browser support now so maybe we should too.

[aweary]

@zpao if it was just the Object.assign() pollyfill then wouldn't this already work with browsers that have a native Object.assign() implementation that supports Symbols? Just curious.

edit: Oh I see, it doesn't actually use the native Object.assign() ever. Would making this module return the native Object.assign() when it's supported resolve this? Or is there a behavior this implements that the native Object.assign() method doesn't?

[zpao]

No, it's a "polyfill" which we always use and never fall back to native implementation. This was because at the time of adding this, the Object.assign spec had just changed (around handling null/undefined I think) so we decided to always use our own code for consistency. Here's the code: https://github.com/facebook/react/blob/22a3d0387b190eed4689937a375ce22b1338803b/src/shared/stubs/Object.assign.js

[aweary]

So would you be open to a PR that checks for symbols as well in assign? I'd be happy to work on it if this is something you guys would like to see.

[zpao]

I don't know yet, would like to get input from @sebmarkbage about intent before deciding how to fix it (personally, I think if yes we would likely just get rid of our polyfill and use babel-polyfill/runtime for it or even just say that you must include a polyfill of your own like we have for ES5 code)

[sebmarkbage]

Yea, we should use native if available. Either using our own module or a mandatory external polyfill. Probably best to keep our own if a native is not available. Easy to miss the polyfill warning if you're only testing in modern browsers.

[aweary]

The Object.assign module could just be updated to return the native Object.assign function if it exists and Symbol support could be added when it doesn't using something similar to what the object-assign pollyfill does

[gingur]

So to be clear, is the purposed solution to eventually rely on the consumer to implement appropriate polyfill (ie https://github.com/zloirock/core-js/blob/v2.2.1/modules/_object-assign.js via babel-polyfill) or for the https://github.com/facebook/react/blob/master/src/shared/stubs/Object.assign.js to be replaced by something like https://github.com/ljharb/object.assign?

[aweary]

@gingur #6376 discusses that, with the later option (using object-assign) being the preferred solution so far.

[gingur]

Would be nice if there was a react-polyfill repo, allowing the consumer to choose to either supply their own pollyfills (ie babel-polyfill) or to use the ones needed for react to run itself. Would also allow node to run react without the need of the polyfills.

[zpao]

Worth noting that there are very few cases of these. We've long used ES5 but ES6 is mostly compiled out or we provide our own polyfills (Object.assign as a module, Object.is inlined are the only 2 I know of). The idea of a react-polyfill repo is interesting though and definitely something we'll keep in mind moving forward.