Remove object-assign polyfill

[gaearon]

Since we already depend on Map and Set I figured we might as well ask users to polyfill Object.assign if they need to.

In browsers, it's still not supported everywhere (e.g. IE 11 doesn’t but Edge does). There is a precedent: Relay Modern, Preact, Inferno don’t ship with it. Many Redux apps use Object.assign so users resort to polyfilling it anyway.

In Node, there was an issue with bad Object.assign implementation in some Node version that caused different attribute order. But Fiber is resilient to that so it wouldn’t be a problem anyway.

Test plan: packaging fixtures work. For internal FB bundles, we didn’t use it in the first place (because we had our own polyfill).

[jquense]

I potential difference in this case from others is that Babel will include its _extends polyfill anyway, it seems like a waste to not use it for this case? It may not be a perfect polyfill but I'd be surprised if that matter here

[jquense]

Talking about this specifically, as a replacement for the extra code. https://babeljs.io/docs/plugins/transform-object-assign/

[gaearon]

Babel will include its _extends polyfill anyway, it seems like a waste to not use it for this case?

Why will Babel include it? I’m not sure I’m following. Do you mean we already have it in our builds? I would expect that not but maybe I’m wrong.

[jquense]

sorry I should re-phrase that :P, its an internal help babel inlines for a few general transforms, You can see it in react-dom@16 doing a find for _extends: https://unpkg.com/[email protected]/umd/react-dom.development.js

Since it's already there, you can use the linked transform to change Object.Assign references to use that instead. It doesn't actually polyfill anything, it works like the plugin you are removing but uses the core babel helper.

[jquense]

The funny thing is that it looks like the Babel transform is deferring to this polyfill lol. It is normally Object.assign || inlineassignhelper, eg. babel inlines the helper then the react babel plugin is replacing it's inlined reference to Object.assign with the object-assign package

[gaearon]

Seems like it's used in just one place, we can probably remove that.

[gaearon]

Or better yet, configure Babel to always use real Object.assign for spread instead of the helper.

[jquense]

sure, easy enough maybe to avoid it, but it seems like an easy win for folks to not have to install another package, add another line to their entry (before React) etc.

On an unrelated note is there an issue tracking warning for things that need to polyfilled? I ran into this trying to test ie9 on the fixture app the other day. I got a hard error for Set and Map missing instead of the more normal "Hey react requires this, here is where you can install a polyfill"

[gaearon]

The plan is to document new required baseline (just like it was the case before ES5 became supported everywhere).

We will probably require Map, Set, requestAnimationFrame (on the client), and Object.assign. But there is no final consensus yet.

Similar to https://facebook.github.io/relay/docs/relay-modern.html#javascript-environment-requirements.

[aweary]

We have an invariant check for requestAnimationFrame, can we do the same here, and for the other required primitives?

[gaearon]

Yeah, we could. Do you want to send a PR?

[Daniel15]

In Node, there was an issue with bad Object.assign implementation in some Node version that caused different attribute order.

No code should be relying on the exact order of object properties though, so this should be safe.

[gaearon]

Our own code was—because we used to compare checksums for markup generated on the server and on the client. But we don’t anymore.

[sebmarkbage]

For inline styles hydration warning we still rely on enumeration order I think. Because we generate a compatible string that we compare against the attribute.

[gaearon]

Ah, I see. Another downside of this is it makes IE11 incompatible which otherwise meets our requirements for minimal target. Let's wait with this until 17.