facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
119.21k stars 24.33k forks source link

Set polyfill says it is [object Object] but should say [object Set] #19594

Open lll000111 opened 6 years ago

lll000111 commented 6 years ago

Environment

Any - you can reproduce the issue using only file

https://github.com/facebook/react-native/blob/master/Libraries/vendor/core/Set.js

and any Javascript encironment.

EDIT: Just for the bot, it is completely irrelevant:

Environment: OS: Windows 10 Node: 10.3.0 Yarn: Not Found npm: 6.1.0 Watchman: Not Found Xcode: N/A Android Studio: Version 3.1.0.0 AI-173.4720617

Packages: (wanted => installed) react: 16.3.1 => 16.3.1 react-native: 0.55.4 => 0.55.4

Description and Steps to Reproduce

Here is code that creates a simple Set object and then uses the spread operator to spread the Set's contents into a new array.

First, using an environment where the above file was executed/loaded (plus its own dependencies), and I also short-circuited the detection at the beginning that determines whether the polyfill is applied:

const s1 = new Set([1,2,3]);
console.log(Object.prototype.toString.call(s1));

Result:

[object Object]

Expected Behavior

Now for comparison a clean ES6+ environment without anything loaded, so we get a native Set implementation:

const s1 = new Set([1,2,3]);
console.log(Object.prototype.toString.call(s1));

Result:

[object Set]
lll000111 commented 6 years ago

Correction (edited main comment: When you test object spread directly it works, but I quote @G-2-Z from somewhere else:

The root cause is that the react-native JavaScript runtime uses a polyfill for Set that doesn't correctly handle calls of Object.prototype.toString.call(mySet). Therefore, testing [...mySet] works as expected but because Object.prototype.toString.call(mySet) returns '[object Object]' instead of '[object Set]' it is never called.

We use a custom JSON-stringifier (deterministic and has support for Map and Set) which uses an object's self-declared type to select the proper stringification, but that's just our anecdotal background story — in any case, the object should identify itself correctly.

react-native-bot commented 6 years ago

It looks like your issue may be missing some necessary information. Can you run react-native info and edit your issue to include these results under the Environment section?

lll000111 commented 6 years ago

@react-native-bot Environment provided (and it still is unnecessary), now remove the label, please, bot....

stale[bot] commented 6 years ago

Hey there, it looks like there has been no activity on this issue recently. Has the issue been fixed, or does it still require the community's attention? This issue may be closed if no further activity occurs. You may also label this issue as "For Discussion" or "Good first issue" and I will leave it open. Thank you for your contributions.

lll000111 commented 5 years ago

Here is what needs to be done: add a method on Symbol.toStringTag to an object that returns the name, here "Map" or "Set".

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/toStringTag

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/@@toStringTag

https://tc39.github.io/ecma262/#sec-object.prototype.tostring ("19.1.3.6 Object.prototype.toString ( )" item 15 "Let tag be ? Get(O, @@toStringTag).")

lll000111 commented 5 years ago

To solve this, this is all that's needed:

    Object.defineProperty(
        Map.prototype,
        Symbol.toStringTag,
        {
            writable: false,
            enumerable: false,
            configurable: true,
            value: 'Map'
        }
    );

    Object.defineProperty(
        Set.prototype,
        Symbol.toStringTag,
        {
            writable: false,
            enumerable: false,
            configurable: true,
            value: 'Set'
        }
    );

You can read more at http://2ality.com/2015/09/well-known-symbols-es6.html#overriding-the-default-tostring-tag

bartolkaruza commented 5 years ago

Hey there, thanks of reporting this! I'm assigning this issue a low priority, because it will not affect the majority of apps. @lll000111 You seem to have a really good handle on the solution, maybe you could help us by submitting a PR?

lll000111 commented 5 years ago

@bartolkaruza I gave you the solution, just copy and paste...

lll000111 commented 5 years ago

@bartolkaruza

Fixing a bug, with a few lines of simple code I already gave you, is "low priority". Fuck this project.

The Object.prototype.toString.call method is frequently used to identify objects and your bug breaks it.

"Low priority" my ass. Good thing I don't use this stuff any more. In the time it took you to post here you could have just fixed the BUG!!! (by copying the code I already provided)

kelset commented 5 years ago

@lll000111 ok, first off, consider this a final warning: use that language again and you will be banned from this repository for breaking the CoC. We won't tolerate this behaviour any further.

Also, you could have literally just submitted a PR with the fix you proposed instead of writing that many comments. This is an Open Source project precisely for this reason.

I'll lock this issue, the only step forward is for anyone to submit a PR that follow the proposed code so that it can be tested and the implementation discussed.