Change Object.assign polyfill to NOT walk entire prototype chain and to NOT force ideologies

[th317erd]

Is this a bug report?

Yes

Have you read the Contributing Guidelines?

Yes

Environment

Environment:
OS: Linux 4.10.0-35-generic #39-Ubuntu SMP Wed Sep 13 07:46:59 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
Node: 8.1.3
npm: 5.3.0

Packages: (wanted => installed)
react-native: 0.49.3 => 0.49.3
react: 16.0.0-beta.5 => 16.0.0-beta.5

Target Platform: Android (7)

Steps to Reproduce

(Write your steps here:)

1. Create an object with keys var a = { test: 'derp' }
2. Create an object with a as the prototype var b = Object.create(a)
3. Attempt to assign b to a new object var c = Object.assign({}, b)

Expected Behavior

react-native shouldn't slowly walk the entire prototype chain or force ideologies

Actual Behavior

react-native DOES slowly walk the entire prototype chain AND force ideologies

Reproducible Demo

https://github.com/facebook/react-native/blob/master/Libraries/polyfills/Object.es6.js

Your Polyfill for Object.assign (Object.es6.js) is completely backwards, slow, and forcing an ideology. You are DELIBERATELY walking and copying the entire prototype chain, which is slow, and often not desired. The SUPER INSANE part? You are telling me how to live my life, and throwing an exception if there IS an enumerable property in the prototype chain. Forcing people to do things YOUR way is the wrong way. Please update the polyfill to 1) NOT walk the entire chain, 2) NOT tell me how to code.

By the way, I ran into this simply inheriting from EventEmitter, which is SUPER COMMON by the way.

I am upset that Facebook has written controlling polyfills forcing ideologies and telling me how to live my life. Please stay off my lawn, stop telling me how to code, and stop doing things the slow way just to tell other people how to do things

// We don't currently support accessors nor proxies. Therefore this
    // copy cannot throw. If we ever supported this then we must handle
    // exceptions and side-effects.
    
    // No SANE person would ever copy an object with `key in obj`
    // please use `Object.keys(obj)` instead, which is much faster,
    // and doesn't involve forced ideologies
    for (var key in nextSource) {
      if (false) { // <--- Add false here... because you don't need to tell me the proper way to code
        var hasOwnProperty = Object.prototype.hasOwnProperty;
        if (!hasOwnProperty.call(nextSource, key)) {
          throw new TypeError(
            'One of the sources for assign has an enumerable key on the ' +
            'prototype chain. Are you trying to assign a prototype property? ' +
            'We don\'t allow it, as this is an edge case that we do not support. ' +
            'This error is a performance optimization and not spec compliant.'
          );
        }
      }
      target[key] = nextSource[key];
    }

[th317erd]

Created a PR for this issue: #16512

[chirag04]

@th317erd thanks for the PR. i suggest changing the title to something more meaningful.

[hramos]

Kind reminder that there is a Code of Conduct in effect. @th317erd, consider this a first warning, and let's make sure any future comments are friendlier and respectful to contributors.

[th317erd]

@hramos I understand that, and appreciate the reminder. Now if you apply the code of conduct to the reason I was upset in the first place you will see the Facebook team violated it in multiple ways by forcing ideologies:

Be considerate: Your work will be used by other people, and you in turn will depend on the work of others.

violated by the code I am complaining about above

Be respectful: Not all of us will agree all the time, but disagreement is no excuse for poor behavior and poor manners.

Yes, I could have had better manners (though I was already restraining my frustration at having my entire app break due to forced ideology and bad coding), but the Facebook team also violated this in respecting other's coding methods and practices... you don't NEED to force people to code a certain way

Be careful in the words that you choose...

I did my best at restraining myself at the time... will try harder in the future

Remember that we’re different. The strength of our community comes from its diversity, people from a wide range of backgrounds.

Again, this is something the Facebook team violated with the above code... we ARE different, and the strength of our community comes from a diversity in genius and thinking outside the box. You forcing people inside a box isn't helping anyone, and I feel is a violation of your own codes of conduct

Again, I will try to have better manners in the future, and I appreciate you listening, and I appreciate your patience with me. I tend to get upset when someone tries to force me into a practice or ideology. Thanks!

[elicwhite]

Please note that I want to help make this satisfy your concerns.

The PR is quite difficult to review and merge confidently, this is true polyfills in general. The following explains why.

I have no context on the initial intent of this code, but I assume that the people who wrote it were smart engineers with more context on the situations it needs to support than I do currently.

The fact that there is a specific warning about the hasOwnProperty checks and that the polyfill is not spec-compliant implies that we started with a spec compliant polyfill and needed to change it to this approach instead. I could also imagine this being an optimization for Prepack, but based on the history of this file not being recent, that likely isn't the case.

// WARNING: This is an optimized version that fails on hasOwnProperty checks
// and non objects. It's not spec-compliant. It's a perf optimization.
// This is only needed for iOS 8 and current Android JSC.

// We don't currently support accessors nor proxies. Therefore this
// copy cannot throw. If we ever supported this then we must handle
// exceptions and side-effects.

It feels like it would be fairly bad form for us to ignore these warnings without better understanding the situations they were meant to solve. Unfortunately, these warnings were not codeified via tests.

Also unfortunately, I couldn't trace the history of this file back to where it was added or modified to be able to see if the original authors remember anything about the reasons. The only people I could find related to that history were @mjesun and @davidaurelio. Perhaps they have some context.

In the mean time, while we are trying to get more context before figuring out how to move forward with those changes, I'd be happy to accept a PR that just contains the additional existence check that you added.

if (!(Object.assign instanceof Function)) {

This check exists on the other polyfills and I can't imagine why we'd want to override an existing Object.assign function.

FWIW, I also have a hard time fully understanding the problems you are trying to address. From you initial post it seems like the main complaint you have is:

shouldn't slowly walk the entire prototype chain or force ideologies

When trying to deconstruct that complaint and understand the specifics, I can understand the first half being performance related. For "force ideologies", I'm not exactly sure what you mean by that. Is that specifically about us overriding existing Object.assign functions which would be satisfied by the addition of the existence check above? Or are there other "ideologies" the code is forcing? Can you help understand the specifics of this?

[th317erd]

@TheSavior Thanks for the feedback. To answer your last question: My complain is 1) Walking the prototype chain while merging objects is slow, and not to standard, 2) Because of 1 there are many instances of code that will break when there ARE enumerable properties in the prototype chain (i.e. inheriting from EventEmitter, which is VERY common Node events). My comment on "forcing ideologies" is related to 1: I should be allowed to have whatever prototype chain I want, and many third party libraries WILL modify the chain with enumerable properties. I should not be FORCED to keep a pure (no enumerable properties) prototype chain, and that prototype chain should not be traversed in the first place.

While if (!(Object.assign instanceof Function)) { is certainly a good addition, I would caution that this AS THE ONLY CHANGE could cause artifacts in development: I.E. When remotely debugging this function most likely WILL exist, but might not exists on native JS engines, meaning the app might function one way while remote debugging, and another way while running on a native JS engine (because the polyfill is non-standard).

Does that help clear things up? Again, thanks for your time and effort on this.

I would certainly be interested on the mentality behind the code in the first place. Let me know what you find out.

[elicwhite]

Since there are very specific uses where the existing code would cause it to break but that code should work, I think the best way for us to understand that and ensure it continues to work would be for you to actually write those cases down as unit tests. It would be good to reproduce those cases in our test suite and ensure that updating the polyfill will solve them. Ideally this would have occurred with the initial changes to the polyfill and if it was we wouldn't be in this situation now. Let's keep our future RN engineers from running into the same problems as us down the road.

Regarding the instanceof check, I'm not sure how the concerns about the differences of engines implies that we should change more of the polyfill at the same time. Also, since all of our other polyfills appear to have that check already, it doesn't seem like those concerns would be unique to this file. For that reason, it seems reasonable to add that separately, earlier.

[th317erd]

@TheSavior "Also, since all of our other polyfills appear to have that check already, it doesn't seem like those concerns would be unique to this file. For that reason, it seems reasonable to add that separately, earlier." I agree it certainly makes sense to add that change to the polyfill (instanceof Function check), however being as the polyfill is currently not to standard I am concerned with switching between platforms that implement the standard, and platforms where the non-standard polyfill will be used. The deviation of the polyfill will potentially cause the app to crash where the polyfill is used (as it does now).

I will write unit tests when I have some free time, which might not be for a few days

[th317erd]

So @TheSavior , I have been thinking about this a lot (haven't had time to come back to it yet to write those tests). I doubt this will break anything. It is functionally identical (pulling only the top-level object keys). The only difference is that, unlike the current polyfill code, it won't check the rest of the prototype chain. So any code that is currently running (not throwing an error due to that check) should behave exactly the same. Since this is throwing an error for everything... it should be fine? The only concern I can think of is if someone did something stu--- I mean "funky" elsewhere that REQUIRED this check not to break, and without the code throwing an exception here maybe it will have issues elsewhere because of enumerable keys in the prototype chain? Anyhow, just a thought... I guess we could always search the code base for any for\s*\(\w+\s+in

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.