chore: enable strict apply, bind, and call #18172
Conversation
mmalerba
added
P2
mmalerba
requested review from
crisbeto,
devversion and
a team
as code owners
googlebot
added
the
cla: yes
| @@ -1842,14 +1842,22 @@ describe('CdkDrag', () => { | |||
| body: document.body, | |||
| fullscreenElement: document.createElement('div'), | |||
| ELEMENT_NODE: Node.ELEMENT_NODE, | |||
| querySelectorAll: function() { | |||
| return document.querySelectorAll.apply(document, arguments); | |||
| querySelectorAll: function(...args: [string]) { | |||
There was a problem hiding this comment.
string[] is a string array of any length, [string] is an array of exactly one string
There was a problem hiding this comment.
If we know it'll be one string, couldn't we just declare it like this?
querySelectorAll: (selector: string) {
return document.querySelectorAll.call(document, selector);
}
There was a problem hiding this comment.
My thinking was that for these ones where we're just wrapping a function and passing things through, the author was trying to be defensive against future browser API changes by just grabbing all of the arguments and shoving them through. This style preserves that
There was a problem hiding this comment.
I'm pretty sure that I'm the author and I think I did it because it was shorter 😄
There was a problem hiding this comment.
I can only speak for the ZoneJS interceptor, where I also did it that way. I wanted to be on the safe side if a different ZoneJS version is used / when the signature changes. The typings are manually copied, so not very reliable IMO.
There was a problem hiding this comment.
Yeah querySelectorAll probably isn't going to change much, but some do, addEventListener has definitely changed over the years
| } | ||
| }, | ||
| querySelectorAll: function() { | ||
| return document.querySelectorAll.apply(document, arguments); | ||
| querySelectorAll: function(...args: [string]) { |
There was a problem hiding this comment.
Also should be string[]? There are a few more changes like this.
jelbourn
added
pr: lgtm
action: merge
(cherry picked from commit 26962a7)
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
cc @mprobst