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

Fix object rest enumeration #29676

Merged
merged 5 commits into from May 7, 2019

Conversation

NicholasLYang
Copy link
Contributor

  • There is an associated issue that is labeled
    'Bug' or 'help wanted' or is in the Community milestone
  • Code is up-to-date with the master branch
  • You've successfully run jake runtests locally
  • You've signed the CLA
  • There are new or updated unit tests validating the change

Fixes #29616

Adjusts the __rest function in destructuring.ts to check if property is enumerable.

@msftclas
Copy link

msftclas commented Feb 1, 2019

CLA assistant check
All CLA requirements met.

@ajafff
Copy link
Contributor

ajafff commented Feb 1, 2019

When this PR lands, the same change should be made to tslib

for (var i = 0, p = Object.getOwnPropertySymbols(s); i < p.length; i++) if (e.indexOf(p[i]) < 0)
t[p[i]] = s[p[i]];
for (var i = 0, p = Object.getOwnPropertySymbols(s); i < p.length; i++) {
if (e.indexOf(p[i]) < 0 && Object.prototype.propertyIsEnumerable(p[i]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect as you are passing the wrong receiver. Its highly unlikely p[i] is enumerable on Object.prototype. You probably mean this:

Suggested change
if (e.indexOf(p[i]) < 0 && Object.prototype.propertyIsEnumerable(p[i]))
if (e.indexOf(p[i]) < 0 && Object.prototype.propertyIsEnumerable.call(s, p[i]))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense. In that case, does s.propertyIsEnumerable(p[i]) work as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I suppose s could have its own implementation of propertyIsEnumerable, so no, that probably does not work.

@rbuckton
Copy link
Member

@rbuckton
Copy link
Member

It looks like you still have some failing tests.

@NicholasLYang
Copy link
Contributor Author

Yep, sorry. Fixed!

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

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.

Object rest includes non-enumerable Symbols
4 participants