-
Notifications
You must be signed in to change notification settings - Fork 788
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(axe.d.ts): allow Node for include/exclude object #3338
Conversation
Co-authored-by: Stephen Mathieson <me@stephenmathieson.com>
typings/axe-core/axe-core-tests.ts
Outdated
@@ -1,7 +1,7 @@ | |||
import * as axe from '../../axe'; | |||
|
|||
var context: any = document; | |||
var $fixture: any = {}; | |||
var $fixture = document.querySelectorAll('div'); |
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'm a little surprised this works, since QSA returns a NodeList rather than an array. Should we explicitly mention NodeList?
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.
Looked into this. It looks like it doesn't matter what exactly is passed in for the include/exclude object so long as it is array-like. Once we get the context object we immediately call parseSelectorArray, which loops over the context include/object object using a for loop that looks at the length of the object to resolve them as VirtualNodes.
I'm OK if we don't mention this particular case for now. We should loop back to this and determine if it's something we explicitly want to support, and if so add an official test to our code to ensure it continues to work.
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.
In that case, wouldn't ArrayLike<Node>
be the best option?
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.
If we decide to officially support it then ya, that should be the correct type. For now I'm going to change the ts test to not use querySelectorAll and will update it back in another pr if we decide to support it (along with the proper test and doc updates).
include?: Node | BaseSelector | Array<Node | BaseSelector | BaseSelector[]>; | ||
exclude?: Node | BaseSelector | Array<Node | BaseSelector | BaseSelector[]>; |
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.
NBD, but it sounds like this should be:
include?: Node | BaseSelector | Array<Node | BaseSelector | BaseSelector[]>; | |
exclude?: Node | BaseSelector | Array<Node | BaseSelector | BaseSelector[]>; | |
include?: Node | BaseSelector | ArrayLike<Node> | Array<BaseSelector | BaseSelector[]>; | |
exclude?: Node | BaseSelector | ArrayLike<Node> | Array<BaseSelector | BaseSelector[]>; |
These all work correctly:
Closes issue: #3334