-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 Array.prototype.copyWithin polyfill #2902
Conversation
@shicks could you review this one, since polyfills are really your thing? |
Created internal issue b/122075328 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long delay - I didn't see this until Bradford pinged me on the internal bug.
@shicks Thanks! I've fixed and commented, please review them. |
I'm not super familiar with the github code review tools - I marked two conversations as unresolved, so hopefully you'll see my responses now. |
assertObjectEquals([1, 3, 4, 4, 5], [1, 2, 3, 4, 5].copyWithin(1, 2, -1)); | ||
}, | ||
|
||
testCopyWithin_throwsIfNullish() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test no longer passes. Please remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? It should be pass and I believe it passes.
this
is null
and this.length
is referred on the top of the polyfill function, so it should throw an error.
https://github.com/google/closure-compiler/pull/2902/files#diff-a5e0f0788d56ad46f3db1223fd3c8b5cR36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look close enough: it fails because the start
and target
arguments are not optional (unfortunately, I don't think there's a way to run the polyfill tests externally, so there's no way for you to reproduce it). The best you can do is something like this:
Array.prototype.copyWithin.call(null, some_other_array, 0);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tjgq understood. It is not a runtime test, but a type check failed. I added the second and the third argument.
}); | ||
}, | ||
|
||
testCopyWithin_throwIfFailToDeleteProperty() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tjgq Probably type check in 2nd arg of Object.defineProperty()
failed, so I fixed it.
@tjgq Thanks! 😸 |
Improvement:
this
is null or undefinedAlso this fixes
.name
and.length
withdefineProperty
.But if the policy of Closure Compiler for them is "won't fix", I will revert them.
I don't know how to run polyfill tests (#1504), but I confirmed this passed all Test262 tests for copyWithin. (https://gist.github.com/teppeis/c8a88f653cdd70ed3e3d6597ec1ee30c)
Ref: