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

Types - Interface Assert, rootElement should also be null #882

Closed
MrChocolatine opened this issue Nov 11, 2020 · 3 comments
Closed

Types - Interface Assert, rootElement should also be null #882

MrChocolatine opened this issue Nov 11, 2020 · 3 comments

Comments

@MrChocolatine
Copy link

MrChocolatine commented Nov 11, 2020

Current signature of the method dom():
https://github.com/simplabs/qunit-dom/blob/6c31d65d825c8c09fab1c4f604b8a89945c06e11/lib/qunit-dom.ts#L10

But it is not completely accurate and triggers an error when rootElement comes from a basic querySelector():

const myTable = this.element.querySelector(/* my-selector */)
assert.dom('tbody > tr', myTable).exists({ count: 2 })
// -------------------------^
// const myTable: Element | null
// Argument of type 'Element | null' is not assignable to parameter of type 'Element | undefined'.
//   Type 'null' is not assignable to type 'Element | undefined'.

rootElement should also be accepted as null:

 dom(target?: string | Element | null, rootElement?: Element | null): DOMAssertions; 
@Turbo87
Copy link
Collaborator

Turbo87 commented Nov 11, 2020

hmm... I'm not sure I agree. If that querySelector() call returned null then what would you expect would happen?

Passing in an element will use that element as the root element. Not passing in anything should default to using the default root element. But explicitly passing in undefined or null seems like a bug to me, no?

@MrChocolatine
Copy link
Author

MrChocolatine commented Nov 11, 2020

I kind of agree @Turbo87 but not entirely.

hmm... I'm not sure I agree. If that querySelector() call returned null then what would you expect would happen?

Nothing. The same that would happen with undefined.

On the other hand, why should we have to do something like this every time to specifically match with the signature?
It seems a bit tedious to me.

const myTable = this.element.querySelector(/* my-selector */)
assert.dom('tbody > tr', myTable || undefined).exists({ count: 2 })

The thing is, *Element | null is literally the common signature of types returned by every* methods/properties to reach a DOM element.
* Okay I did not check absolutely every methods and properties but querySelector(), getElementById(), ownerElement, activeElement ... The list goes on.

@MrChocolatine
Copy link
Author

MrChocolatine commented Jan 12, 2021

Any update on this please :)?

The type returned by querySelector() being Element | null (like many other DOM methods), it does not match the types of the second argument rootElement of assert.dom().

But explicitly passing in undefined or null seems like a bug to me, no?

Even if, in the context of a test, we know our element should exist me might have made a mistake in the selector and null would be implicitly returned (then the default root element would be used), we should not have to do anything more with the following code:

const myElem = this.element.querySelector(/* my-selector */)
assert.dom('.some-class', myElem).exists({ count: 2 })

@MrChocolatine MrChocolatine changed the title Types - Interface Assert, rootElement can also be null Types - Interface Assert, rootElement should also be null Jan 12, 2021
@MrChocolatine MrChocolatine closed this as not planned Won't fix, can't repro, duplicate, stale Feb 2, 2023
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

No branches or pull requests

2 participants