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

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

Open
lll000111 opened this issue Jun 6, 2018 · 10 comments
Open

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

lll000111 opened this issue Jun 6, 2018 · 10 comments

Comments

@lll000111
Copy link

lll000111 commented Jun 6, 2018

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
Copy link
Author

lll000111 commented Jun 6, 2018

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.

@lll000111 lll000111 reopened this Jun 6, 2018
@lll000111 lll000111 changed the title Set polyfill produces incorrect result when using [...mySet] (spread) Set polyfill says it is [object Object] but should say [object Set] Jun 6, 2018
@react-native-bot

This comment has been minimized.

@lll000111

This comment has been minimized.

@stale
Copy link

stale bot commented Sep 30, 2018

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
Copy link
Author

lll000111 commented Jan 16, 2019

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
Copy link
Author

lll000111 commented Jan 30, 2019

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
Copy link

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

This comment has been minimized.

@lll000111

This comment has been minimized.

@kelset
Copy link
Contributor

kelset commented Mar 22, 2019

@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.

@facebook facebook locked as too heated and limited conversation to collaborators Mar 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants