Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object.assign polyfilled in environment #1998

Closed
syranide opened this issue Aug 5, 2014 · 10 comments
Closed

Object.assign polyfilled in environment #1998

syranide opened this issue Aug 5, 2014 · 10 comments
Milestone

Comments

@syranide
Copy link
Contributor

syranide commented Aug 5, 2014

https://github.com/facebook/react/blob/master/src/vendor/polyfill/Object.es6.js#L23
https://github.com/facebook/react/blob/master/src/browser/ui/React.js#L23

This goes against our "policy" (AFAIK) for dealing with polyfills/shims, if we want to ship the polyfill it should be returned and not shim the global Object.

@syranide
Copy link
Contributor Author

syranide commented Aug 5, 2014

cc @zpao @spicyj

@zpao
Copy link
Member

zpao commented Aug 5, 2014

I thought I had another issue here somewhere about this but can't find it. We're probably going to need to cave and ship something that modifies the global Object. We'll have a better story by the time we ship 0.12.

@zpao zpao added this to the 0.12 milestone Aug 5, 2014
@bloodyowl
Copy link
Contributor

what prevents you to ship it as a module exports which returns whether the native Object.assign or the polyfill?

@zpao
Copy link
Member

zpao commented Aug 6, 2014

The whole point of polyfills is that you use the same syntax you would if it was supported in the environment. Object.assign(...), not require('objassign').assign(...). Otherwise you need to teach everybody to use that module. And then change all of your code when it's natively supported.

We're also going to start supporting some native ES7 syntax in our transforms which will desugar to use Object.assign.

Eg

var a = {foo: 1};
var b = {...a, bar: 2};
b; // {foo: 1, bar: 2}

// transformed becomes
var b = Object.assign({}, a, {bar: 2});

@sophiebits
Copy link
Collaborator

For something like Object.assign it's possible that we could pull out all the calls at build time but for things that modify built-in prototypes (like Function.prototype.bind) it's not.

@syranide
Copy link
Contributor Author

syranide commented Aug 6, 2014

I assume we just shouldn't ship Object.assign with React (as with all other polyfills), but there's no obvious/suitable package/library right now?

@zpao
Copy link
Member

zpao commented Aug 6, 2014

@syranide that's what we need to figure out before we ship 0.12. It might make more sense to ship this polyfill since it's still new (spec changed last week) and we can control distribution a little bit better. It would definitely be a change for us. There's a problem looming in the likely-not-too-distant future where Maps and Sets are used and we have a bigger polyfill problem.

@syranide
Copy link
Contributor Author

syranide commented Aug 6, 2014

If we will require support for non-string indices... yes, that would be an issue. :)

You can always fall back to an implementation that mutates non-string keys, which may be acceptable where the injected property could be made non-enumerable, but that's not supported by the oldest of browsers (IE8, and perhaps the Andriod 2.x browser?), cloning might be an issue? For the oldest browsers, I imagine that it would be possible to hijack a default non-enumerable method and replace it with a pass-through function, but one which we could inject a property into without fear.

Or what's the plan?

@zpao
Copy link
Member

zpao commented Aug 6, 2014

We'll figure out the details before we ship.

@zpao zpao mentioned this issue Oct 8, 2014
3 tasks
@zpao
Copy link
Member

zpao commented Oct 15, 2014

Not going to polyfill, will just use our own impl and not depend on Object.assign existing. #2350

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

No branches or pull requests

4 participants