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

FEAT: Implement Symbol.unscopables #1556

Merged

Conversation

0xe
Copy link
Contributor

@0xe 0xe commented Aug 7, 2024

Implements Array.prototype[Symbol.unscopables] from the spec.

@0xe 0xe force-pushed the scratch/satish/add-symbol-unscopables-nativearray branch from 4ee8718 to 343d41f Compare August 7, 2024 09:42
@rbri
Copy link
Collaborator

rbri commented Aug 7, 2024

LGTM 👍

@rbri
Copy link
Collaborator

rbri commented Aug 7, 2024

do we need a feature check for ES6 here?

@gbrail
Copy link
Collaborator

gbrail commented Aug 8, 2024

Looks good and thanks! Can you please re-base on top of the latest to pick up the changes to test262?

@0xe
Copy link
Contributor Author

0xe commented Aug 8, 2024

do we need a feature check for ES6 here?

I think we don't initialise Symbol support unless it's ES6. So evaluating any Symbol throws ReferenceError in non-es6 which seems to be expected behaviour. I was following what we are doing to Symbol.species.

The spec doesn't seem to talk about throwing an different error in older versions when the Symbols are referenced.

@0xe 0xe force-pushed the scratch/satish/add-symbol-unscopables-nativearray branch from 343d41f to 0c1b2a1 Compare August 8, 2024 18:21
@gbrail
Copy link
Collaborator

gbrail commented Aug 9, 2024

Thanks, this makes sense. Any idea why that one remaining test continues to fail -- is it something we can address in another PR?

prototype/Symbol.unscopables/change-array-by-copy.js

@0xe
Copy link
Contributor Author

0xe commented Aug 9, 2024

Thanks, this makes sense. Any idea why that one remaining test continues to fail -- is it something we can address in another PR?

prototype/Symbol.unscopables/change-array-by-copy.js

This test uses for (const unscopable of [...]) loop which doesn't work in Rhino (we throw syntax error for this form). The same check works if rewritten using for (var unscopable of [...]) like this:

var unscopables = Array.prototype[Symbol.unscopables];

for (var unscopable of ["toReversed", "toSorted", "toSpliced"]) {
    verifyProperty(unscopables, unscopable, {
        value: true,
        writable: true,
        configurable: true
    })
};

assert(!Object.prototype.hasOwnProperty.call(unscopables, "with"), "does not have `with`");

@p-bakker p-bakker linked an issue Aug 10, 2024 that may be closed by this pull request
@gbrail
Copy link
Collaborator

gbrail commented Aug 12, 2024

OK, thanks for the explanation of the test -- looks good to me.

@gbrail gbrail merged commit 93a893e into mozilla:master Aug 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ES2015 Symbol.unscopable behavior
3 participants