Update typings for convert

[Roang-zero1]

The current typings for the convert function argument do not include undefined or null, even though these cases are handled.

Adding these types would allow users to not implement additional checks, but let unist-util-is handle the verification.

[ChristianMurphy]

Thanks @Roang-zero1!
Could you add some typing tests, verifying how the updated Test type impacts the is function?

[Roang-zero1]

Thanks @Roang-zero1!
Could you add some typing tests, verifying how the updated Test type impacts the is function?

Sure. Question in that regard:
In the current type tests is<Node>(heading) is an error due to a missing test, but in the Readme is(node) is a valid test.
Which version is correct?

[wooorm]

The types are wrong. It’s useful and normal to be able to check if something is a node

[Roang-zero1]

Updated test parameter to be optional and added tests.

[Rokt33r]

In my opinion, adding undefined and null to Test type looks quite wrong. It should just define how test looks like. So other libraries like unist-util-find don't allow null or undefined.

So I think we should fix argument type of the functions.

cc @ChristianMurphy

[Rokt33r]

Should be test?: unistUtilIs.Test<T> | Array<unistUtilIs.Test<any>> | null. And don't forget to fix convert function too.

[ChristianMurphy]

This is needed only if null and undefined are removed from Test correct?

[Rokt33r]

Yes.

[ChristianMurphy]

In my opinion, adding undefined and null to Test type looks quite wrong. It should just define how test looks like.

I see where you're coming from on this.
Another way of looking at it is null and undefined are universal selectors/tests.

So other libraries like unist-util-find don't allow null or undefined.

unist-util-find appears to be the exception, looking through the unist-utils, many of the others including:

• https://github.com/syntax-tree/unist-util-filter
• https://github.com/syntax-tree/unist-util-find-after
• https://github.com/syntax-tree/unist-util-find-all-after
• https://github.com/syntax-tree/unist-util-find-all-before
• https://github.com/syntax-tree/unist-util-find-before
• https://github.com/syntax-tree/unist-util-index
• https://github.com/syntax-tree/unist-util-size
• https://github.com/syntax-tree/unist-util-visit
• https://github.com/syntax-tree/unist-util-visit-parents

do support null and undefined, it feels off doing the same customization in 9 typings.

[wooorm]

unist-util-find hasn’t been updated in years, and isn’t maintained by the collective, so it’s not the best of examples!

[Rokt33r]

Another way of looking at it is null and undefined are universal selectors/tests.

@wooorm so do you agree with this? If so, rather than removing null and undefined from Test, I think we should update document that they are considered as universal selectors/tests

[wooorm]

Right, yeah, the given test is optional (and null and undefined mark it as such explicitly). No test means that a test is created that checks if the later given value is a node. It’s documented here: “When not given, checks if node is a Node.”

If this should be clearer, what do you suggest?

[Rokt33r]

I think the document should describe what can be a Test. is is accepting Array<Test> but there is no explicit description about Test.

But I still think removing undefined and null from Test is clearer. In my opinion, "When not given, checks if node is a Node." is just behavior of is when adopters doesn't provide any tests.

But the actual effect of the removing is quite limited. It is just about allowing [null, ...] and [undefined, ...] or not.

[wooorm]

I think the document should describe what can be a Test. is is accepting Array but there is no explicit description about Test.

All cases of the test parameter are described in the readme? https://github.com/syntax-tree/unist-util-is#isnode-test-index-parent-context

But I still think removing undefined and null from Test is clearer. In my opinion, "When not given, checks if node is a Node." is just behavior of is when adopters doesn't provide any tests.

I see it the other way around: is checks if values are a node. If you give a non-nully value, it checks that the node is a certain kind of node.

Note: I’m not talking about typings: I’m discussing the JS library, maybe that’s why we’re missing each other’s points?

[Rokt33r]

I’m not talking about typings: I’m discussing the JS library, maybe that’s why we’re missing each other’s points?

I think so. I just want to make the types based on the document.

All cases of the test parameter are described in the readme?

Yes, they are. But the document about Test in Array.<Test> still feels a bit unclear to define types. I just want to grab your idea about Test.

[ChristianMurphy]

Yes, they are. But the document about Test in Array.<Test> still feels a bit unclear to define types.

How so?

[ChristianMurphy]

🔔 @Rokt33r

[Rokt33r]

Yes, they are. But the document about Test in Array. still feels a bit unclear to define types.

How so?

For me, null or undefined just means empty value. For me, using those values means "I don't have any tests to use" rather than "I provide an empty test".

So when I read Function, string, Object, or Array.<Test>, optional, I assumed Array.<Test> means Array<TestFn | string | {}>. Also When not given, checks if node is a Node., in the readme, is just explaining the behavior of is not the type. And optional just means that adopters don't need to provide any value to the test parameter(if they want to check if node is a Node). So it doesn't sound the nully values is a Test for me.

Also allowing [null] or [undefined] feels meaningless to me.

Btw, I have another suggestion. Why don't we just make another type alias, Testable.

type Test<T extends Node> =
  | TestType<T>
  | TestObject<T>
  | TestFunction<T>

type Testable<T extends Node> = Test<T> | Array<Test<T>> | undefined | null

It would save the hustle you mentioned in #11 (comment)

[ChristianMurphy]

For me, using those values means "I don't have any tests to use" rather than "I provide an empty test".

🤷‍♂️ I see it both ways.
A bit more the empty test way.

---

type Test<T extends Node> =
  | TestType<T>
  | TestObject<T>
  | TestFunction<T>

type Testable<T extends Node> = Test<T> | Array<Test<T>> | undefined | null

It would save the hustle

It would reduce the hustle, but we'd still need to go to each package and update Test to Testable, and would need to communicate to future adopters what the difference between the two is.

---

Another idea,
maybe keep undefined and null in the Test type, and document that it can be used as NonNullable<Test> when it should not include null or undefined.
https://www.typescriptlang.org/docs/handbook/utility-types.html#nonnullablet

Thoughts? 💭

[Rokt33r]

It would reduce the hustle, but we'd still need to go to each package and update Test to Testable

Hmm I'm also thinking that we cannot justify avoiding the hustle if there are some better ways to do. Anyway, let's try to find a consensus first. Once it is determined and is required more hustles or less, I would gladly do.

maybe keep undefined and null in the Test type, and document that it can be used as NonNullable when it should not include null or undefined.

Using NonNullable sounds good to me. But it cannot justify to keep nully values.

Anyway, the only problem I do have is that we didn't define what Test is explicitly in the documentation. So it makes us to guess its definition and the guessing is causing the misunderstanding we have now. So I would be fine to add nully types to Test as long as we define Test explicitly in our specs(the documentation).

But I'm sure we don't expect users to use [null] because it is just meaningless. And Test is only used in Array.<Test> in our documentation. That's the reason why I think Test doesn't include nully values.

Maybe we should not use Test for the parameter type because Test is only used for the array in our documentation.

So it should be like

test?: TestType<T>
  | TestObject<T>
  | TestFunction<T>
  | null
  | Array<unistUtilIs.Test<any>>

Otherwise, we should update our documentation like below

test (Test, or Array.<Test>, optional) — Test can be Function, string, Object or null When not given or null, checks if node is a Node. ....

Then we can definitely include nully values to Test.

[wooorm]

This seems to have stalled, and I’m not how to continue this?

[Rokt33r]

@wooorm Sorry for my late answer. I'm still thinking that we should not include null if we don't update the definition of Test in Readme.

[wooorm]

cc/ @ChristianMurphy, what about you?

I mainly want to resolve this. Either docs, or types, or whatnot.

[ChristianMurphy]

#11 (comment) still summarizes my thoughts on this.
I see null as a special type of type test and as something we may want to document and include in the typing.

[ChristianMurphy]

Put another way, if we add null to type Test, this PR is semver minor, all plugins that depend on it get the update automatically, which matches their expected behavior #11 (comment).
If we split the type as suggested in #11 (comment), this becomes semver major, as we now need to update every typing that depends on Test.

[wooorm]

Thanks @Roang-zero1 for your patience.

I believe adding this, plus the note I added to the docs, makes the types, docs, and actual implementation, as close to each other as possible, and as explicit as can be without breaking everything.