Skip to content

Remove object-assign polyfill#10280

Closed
gaearon wants to merge 1 commit into
react:masterfrom
gaearon:no-oo-polyfill
Closed

Remove object-assign polyfill#10280
gaearon wants to merge 1 commit into
react:masterfrom
gaearon:no-oo-polyfill

Conversation

@gaearon

@gaearon gaearon commented Jul 26, 2017

Copy link
Copy Markdown
Collaborator

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.

screen shot 2017-07-26 at 12 53 07 pm

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

screen shot 2017-07-26 at 12 54 49 pm

@jquense

jquense commented Jul 26, 2017

Copy link
Copy Markdown
Contributor

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

jquense commented Jul 26, 2017

Copy link
Copy Markdown
Contributor

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

@gaearon

gaearon commented Jul 26, 2017

Copy link
Copy Markdown
Collaborator Author

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

jquense commented Jul 26, 2017

Copy link
Copy Markdown
Contributor

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/react-dom@16.0.0-alpha.13/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

jquense commented Jul 26, 2017

Copy link
Copy Markdown
Contributor

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

gaearon commented Jul 26, 2017

Copy link
Copy Markdown
Collaborator Author

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

@gaearon

gaearon commented Jul 26, 2017

Copy link
Copy Markdown
Collaborator Author

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

@jquense

jquense commented Jul 26, 2017

Copy link
Copy Markdown
Contributor

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

gaearon commented Jul 26, 2017

Copy link
Copy Markdown
Collaborator Author

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

aweary commented Jul 26, 2017

Copy link
Copy Markdown
Contributor

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

@gaearon

gaearon commented Jul 26, 2017

Copy link
Copy Markdown
Collaborator Author

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

@Daniel15

Copy link
Copy Markdown
Contributor

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

gaearon commented Jul 27, 2017

Copy link
Copy Markdown
Collaborator Author

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.

@bvaughn bvaughn mentioned this pull request Aug 1, 2017
@sebmarkbage

sebmarkbage commented Aug 1, 2017

Copy link
Copy Markdown
Contributor

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

gaearon commented Aug 1, 2017

Copy link
Copy Markdown
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants