Fix type assertion #3
Conversation
github-actions
bot
added
👋 phase/new
There was a problem hiding this comment.
It looks like there is a failing test 🤔
not ok 23 should call `test` if the given node is a valid element (2)
---
operator: equal
expected: false
actual: true
stack: |-
Error: should call `test` if the given node is a valid element (2)
at Test.assert [as _assert] (/home/runner/work/hast-util-is-element/hast-util-is-element/node_modules/tape/lib/test.js:311:54)
at Test.bound [as _assert] (/home/runner/work/hast-util-is-element/hast-util-is-element/node_modules/tape/lib/test.js:96:32)
at Test.strictEqual (/home/runner/work/hast-util-is-element/hast-util-is-element/node_modules/tape/lib/test.js:475:10)
at Test.bound [as equal] (/home/runner/work/hast-util-is-element/hast-util-is-element/node_modules/tape/lib/test.js:96:32)
at Test.<anonymous> (file:///home/runner/work/hast-util-is-element/hast-util-is-element/test.js:153:8)
at Test.bound [as _cb] (/home/runner/work/hast-util-is-element/hast-util-is-element/node_modules/tape/lib/test.js:96:32)
at Test.run (/home/runner/work/hast-util-is-element/hast-util-is-element/node_modules/tape/lib/test.js:114:31)
at Test.bound [as run] (/home/runner/work/hast-util-is-element/hast-util-is-element/node_modules/tape/lib/test.js:96:32)
at Test._end (/home/runner/work/hast-util-is-element/hast-util-is-element/node_modules/tape/lib/test.js:217:11)
at Test.bound [as _end] (/home/runner/work/hast-util-is-element/hast-util-is-element/node_modules/tape/lib/test.js:96:32)
...
https://github.com/syntax-tree/hast-util-is-element/pull/3/checks?check_run_id=3882934738#step:5:51
Checking, I don't get this error on gitpod.io |
|
@ChristianMurphy It should be working now. |
|
ping @wooorm |
| @@ -56,8 +61,10 @@ export const isElement = | |||
| * When a `parent` node is known the `index` of node should also be given. | |||
| * | |||
| * @type {( | |||
| * (<T extends Element>(node: unknown, test: T['tagName']|TestFunctionPredicate<T>|Array.<T['tagName']|TestFunctionPredicate<T>>, index?: number, parent?: Parent, context?: unknown) => node is T) & | |||
| * ((node?: unknown, test?: Test, index?: number, parent?: Parent, context?: unknown) => boolean) | |||
| * (() => false) & | |||
There was a problem hiding this comment.
I still think we should remove this in typings.
There was a problem hiding this comment.
What is the benefit in removing it?
The benefit in having it is that it matches how the code works. Generally, types should match the code. Why deviate from the code and introduce a breaking change?
There was a problem hiding this comment.
Generally, types should match the code.
TypeScript should be stricter than codes, the benefit is that isElement() is always meaningless in practice, and you should not use it at all. For example, parseInt(num) or parseInt() are valid in js, but error in ts.
introduce a breaking change
Personally, I won't say updating better practice types as breaking change.
|
@wooorm Please help to review again. |
|
ping @wooorm |
| @@ -56,8 +61,10 @@ export const isElement = | |||
| * When a `parent` node is known the `index` of node should also be given. | |||
| * | |||
| * @type {( | |||
| * (<T extends Element>(node: unknown, test: T['tagName']|TestFunctionPredicate<T>|Array.<T['tagName']|TestFunctionPredicate<T>>, index?: number, parent?: Parent, context?: unknown) => node is T) & | |||
| * ((node?: unknown, test?: Test, index?: number, parent?: Parent, context?: unknown) => boolean) | |||
| * (() => false) & | |||
There was a problem hiding this comment.
What is the benefit in removing it?
The benefit in having it is that it matches how the code works. Generally, types should match the code. Why deviate from the code and introduce a breaking change?
Co-authored-by: Titus <tituswormer@gmail.com>
Co-authored-by: Titus <tituswormer@gmail.com>
This comment has been minimized.
This comment has been minimized.
|
Thanks, released! |
wooorm
added
☂️ area/types
close #2
Initial checklist
Description of changes
As title