Use object-assign instead of our own dated "polyfill"

[zpao]

Not 100% sure if we should do this and add to the list of "where does this come from" modules. Simply rewriting and requiring the defacto polyfill instead of our own. This will require a few small changes internally.

A couple alternatives:

• change all require('Object.assign') to require('object-assign'). Same amount of work, but potentially clearer that this is the npm module and now a providesModule. This will play nicely into Get rid of providesModule #6336 when that happens.
• Just update our Object.assign to be the content of object-assign (and add copyright headers, etc). This is the least amount of work but a bit like cheating :).

Net result for browser packages is the same for all of these - we end up with object-assign packages. The only real difference would be in the npm package and adding another dependency.

Fixes #6372

[gaearon]

You probably know which option I like 😄 .

change all require('Object.assign') to require('object-assign'). Same amount of work, but potentially clearer that this is the npm module and now a providesModule. This will play nicely into #6336 when that happens.

[quantizor]

Just out of curiosity, why use Object.assign vs enabling the babel spread operator and using that?

[aweary]

@yaycmyk object spread is just syntactic sugar for Object.assign with babel (example)

[quantizor]

@aweary I'm aware of that. My suggestion is to use the babel version since that means eventually you'll be able to turn the transform off without having to rip out require references all over the codebase.

[aweary]

@yaycmyk from what I can see the polyfill that babel uses when Object.assign isn't available doesn't support Symbols (which object-assign does, see #6372) and that would require going in and changing all assign calls while this change only requires a change in the build step.

[sebmarkbage]

Wouldn't it make more sense to have this redirect live in fbjs? E.g. a forwarding module. That's the place where we swap out FB specific polyfills for community ones.

[facebook-github-bot]

@zpao updated the pull request.

[sebmarkbage]

Why are we picking object-assign specifically? What do babel code end up depending on? fbjs has a core-js dependency. Can we end up with one?

My only concern for this is perf. We know that we've intentionally overridden this in RN in the past because it is too slow.

How do we override this internally?

[gaearon]

Babel desugars spread operator to this helper:

"use strict";

var _extends = Object.assign || function (target) { for (var i = 1; i < arguments.length; i++) { var source = arguments[i]; for (var key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } } return target; };

However if you import babel/polyfill, you’ll actually end up with Object.assign() as implemented by core-js.

[zpao]

I picked object-assign because it's stable and apart from the Symbol support, virtually identical to what we have now. Compare https://github.com/sindresorhus/object-assign/blob/master/index.js to https://github.com/facebook/react/blob/05b05c4c81154b6e653a10479dd77a569d2b52f7/src/shared/stubs/Object.assign.js

In RN if perf is a concern then they can set Object.assign before loading any other modules and they'll be better off anyway (will catch the user code too). Since this module actually uses Object.assign first instead of always using a custom method that means RN only has 1 thing to override.

on www we would have

// @providesModule object-assign
module.exports = Object.assign;

This is what we do already for @providesModule Object.assign (or if we did exactly as I have already here without rewriting requires, it would be identical). This will use our real polyfill which uses the native impl if it exists. This means www already supports Symbols.

As for fbjs… in theory sure. But React is pretty much the only thing internally using this Object.assign module. Everything else uses Object.assign or spread directly.

---

I chose not to use the babel transform / runtime since I didn't want to make another direct dependency on core-js which isn't the smallest dep. And inlining _extends into all the callsites wouldn't be great for bytesize. If we went through fbjs for this then yes, it would be a transitive dep which is already there so wouldn't be too bad. But see previous note about that.

[facebook-github-bot]

@zpao updated the pull request.

[mridgway]

What is the plan going forward with regard to requiring the users to polyfill ES6 features themselves? Is there a benchmark that should be met for React to change the base requirement from ES5 to ES6?

[zpao]

There isn't a solid plan, we've mostly taken them on a case by case basis. We've inlined in a couple places (eg Object.is because it's a single line and there would have been one or 2 callsites). Another instance of Symbol support was just to get a Symbol in 1 place so we did an inline check and fallback to a string.

ES5 was easy to make the call and just tell people to bring their own. Personally I mostly feel the same about ES6, with some careful consideration about using things that can't be polyfilled. I don't know what the benchmark is to get everybody to agree with me though :) Historically Node made that harder to really say but with the recent upgrade rate it's much more doable. Polyfilling a Node environment is still pretty gross though and I don't think I could comfortably argue that everybody doing server rendering must require('core-js/path/to/es6/polyfil') before running any code.

[gingur]

Might I suggest my comment #6372 (comment) to be a possible solution going forward?

[facebook-github-bot]

@zpao updated the pull request.

[zpao]

FYI: I'm cleaning up other callers internally and in RN (#6376) so I think I might change this slightly so that we are using the object-assign module directly instead of redirecting like this. It's a bit more work but it feels better.

[gaearon]

in RN (#6376)

Probably facebook/react-native#6766.

[facebook-github-bot]

@zpao updated the pull request.

[zpao]

Took it one step further and replaced all uses of our own assign with Object.assign directly. I wrote a Babel plugin (based on the idea of http://babeljs.io/docs/plugins/transform-object-assign/ but inserting requires instead of the _extends helper for less code and an actual Object.assign polyfill). I could have used file.addImport but that ends up generating the interopRequire noise which I didn't want. So at the end of the day our generated code is virtually identical to what we have right now.

// before
var assign = require('Object.assign');
assign(...);

// after
Object.assign(...);

// after transform
var _assign = require('object-assign');
_assign(...)

[zpao]

@sebmarkbage accepted verbally the other day so just updating the label to match that.

[facebook-github-bot]

@zpao updated the pull request.

[mijamo]

I don't know if this is the right place to say it but in the future it would be nice to avoid code removals between the last RC and the final version. With RC1 and 2 everything was working perfectly fine but with the final version some (pretty popular as you can see above) React components broke because they relied on a react polyfill that did not exist anymore. IMHO it would have deserved a RC3 to give time for the maintainers to adapt instead of breaking the code without previous notice. I know it is hard to imagine all the possible outcomes but removing an internal function might always have negative consequences that should be prevented before the launch.

[gaearon]

@mijamo Yes, doing this was a mistake. We shouldn’t do this again. cc @zpao

[aweary]

@gaearon could we export object-assign from react/lib/Object.assign wrapped in a warning to prevent this for now?

[gaearon]

Oh, I was referring to removing React.__spread and the TypeScript issue.

While react/lib/Object.assign story is similar, I don’t think we’ll be adding it back. I believe we explicitly said many times that components must never rely on anything inside react/lib/.

React.__spread kinda was part of the public API, at least to the JSX transforms, but was never the case for React/lib/Object.assign. We could potentially remove it in any patch version. We really can’t make any guarantees about internal modules people choose to rely on because they can be gone after any refactoring.

[zpao]

We won't be re-adding that module. None of the lib/ files are part of our public API and using them has always been at your own risk. We move and rename files with some frequency.

We're reverting the __spread change because it was on the exported object and even though it wasn't a supported API, it was there.

IMHO it would have deserved a RC3

You're right, we should have done an RC3.

[sebmarkbage]

In a way... Causing havoc is sometimes a good thing. It helps enforce expectations. We should avoid if possible though.

On Apr 8, 2016, at 9:13 AM, Paul O’Shannessy [email protected] wrote:

We won't be re-adding that module. None of the lib/ files are part of our public API and using them has always been at your own risk. We move and rename files with some frequency.

We're reverting the __spread change because it was on the exported object and even though it wasn't a supported API, it was there.

IMHO it would have deserved a RC3

You're right, we should have done an RC3.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub