Address issues with typings

[JLRishe]

This addresses some issues with the recent type changes:

• The select() functions return a Node array, string, number, or boolean, but as of now, the .d.ts has them returning a Node array, Node, string, boolean, or null
• In the context of this package, the nodetype functions only really make sense when used on the result of a select1() call, so I have updated them to reflect that. Likewise, the isNodeArray() function makes sense when called on the result of a select() call.

[JLRishe]

@cjbarth @lukeggchapman Please review and provide feedback.

[cjbarth]

I don't think this is entirely correct. These typing changes result in select(_, _, false)[0] returning a Node but select1() can possibly return null. Given that select1() is just select(_, _, true), which in itself is just select(_, _, false)[0], we have the same result possibly being two different types according to this. What am I missing?

[JLRishe]

The only situation where select(_, _, true) would return null (or rather undefined) is if it were evaluating a nodeset expression that did not match any nodes.

So in that situation,

select(_, _, false) is Array<Node> of length 0.

select(_, _, false)[0] is undefined, since any nonexistent index on an array is undefined.

select(_, _, true) is undefined.

So there is no real inconsistency, though it seems that the definition of SelectSingleReturnType should use undefined instead of null.

[cjbarth]

If you look at selectWithResolver(), which all these eventually call, it appears that any given XPath can return a string, number, boolean, or result.nodes in the case of false, or results.nodes[0] in the case of true. I see no check to make sure that result.nodes is not undefined, which seems to mean that if true, then an error would be thrown trying to index [0] of undefined, or perhaps array with .length === 0.

This means that false can, in fact, return a ScalarValue. Does that sound right? What is more, I'm not sure why ScalarValue can't include null as I would expect that there are valid XPath expressions that would legitimately return null or undefined against an XML document. Or, would they just return a results.nodes with a .length === 0?

[JLRishe]

In the XPath 1.0 data model, an XPath expression can only evaluate to one of four types, and barring any ill-defined custom functions, any given expression will always evaluate to the same type:

• string
• number
• boolean
• nodeset

I see no check to make sure that result.nodes is not undefined

The function you are referring to contains a series of if statements that check whether the result is a string, number, or boolean. If it is none of these, then by process of elimination, it is a nodeset, and has a .nodes property that is an array. the process that ultimately leads to this is convoluted, but I can say with a very high degree of confidence that this will always be the case.

then an error would be thrown trying to index [0] of undefined, or perhaps array with .length === 0.

Accessing the [0] index on an empty array does not result in an error. It simply evaluates to undefined

This means that false can, in fact, return a ScalarValue. Does that sound right?

Yes, my modification in this PR accounts for this.

I'm not sure why ScalarValue can't include null as I would expect that there are valid XPath expressions that would legitimately return null or undefined against an XML document.

The XPath 1.0 data model has no concept of a null result. An expression can only evaluate to one of the four types listed above. This package includes convenience functions which, in the case that an XPath evaluates to a nodeset, return the first node in that nodeset. And given that a nodeset can be empty, these convenience functions can return undefined, but the actual result type of an XPath expression can never be null or undefined.

[cjbarth]

Thank you for that thorough explanation. It seems that SelectSingleReturnType should then be typed to return undefined. After you push that change, I'll check this against xml-crypto and let you know how that goes.

[JLRishe]

Made the change. Thanks.

[cjbarth]

In testing this in xml-crypto it seems that it would be helpful to take in null and undefined as arguments, this way I don't have to have multiple checks.

Suggested change

	export function isNodeLike(value: SelectSingleReturnType): value is Node;
	export function isArrayOfNodes(value: SelectReturnType): value is Node[];
	export function isElement(value: SelectSingleReturnType): value is Element;
	export function isAttribute(value: SelectSingleReturnType): value is Attr;
	export function isTextNode(value: SelectSingleReturnType): value is Text;
	export function isCDATASection(value: SelectSingleReturnType): value is CDATASection;
	export function isProcessingInstruction(value: SelectSingleReturnType): value is ProcessingInstruction;
	export function isComment(value: SelectSingleReturnType): value is Comment;
	export function isDocumentNode(value: SelectSingleReturnType): value is Document;
	export function isDocumentTypeNode(value: SelectSingleReturnType): value is DocumentType;
	export function isDocumentFragment(value: SelectSingleReturnType): value is DocumentFragment;
	export function isNodeLike(value?: SelectSingleReturnType | null): value is Node;
	export function isArrayOfNodes(value?: SelectReturnType | null): value is Node[];
	export function isElement(value?: SelectSingleReturnType | null): value is Element;
	export function isAttribute(value?: SelectSingleReturnType | null): value is Attr;
	export function isTextNode(value?: SelectSingleReturnType | null): value is Text;
	export function isCDATASection(value?: SelectSingleReturnType | null): value is CDATASection;
	export function isProcessingInstruction(value?: SelectSingleReturnType | null): value is ProcessingInstruction;
	export function isComment(value?: SelectSingleReturnType | null): value is Comment;
	export function isDocumentNode(value?: SelectSingleReturnType | null): value is Document;
	export function isDocumentTypeNode(value?: SelectSingleReturnType | null): value is DocumentType;
	export function isDocumentFragment(value?: SelectSingleReturnType | null): value is DocumentFragment;

[JLRishe]

The definition of SelectSingleReturnType already includes undefined, so it doesn't seem like there's a need to re-indicate that in the function signatures. SelectReturnType doesn't include undefined, but I'm not sure I can see a good case for having isArrayOfNodes accept undefined.
And ? is for indicating a parameter that can be omitted completely, so that wouldn't be appropriate here.

I think I can see the value of having these function accept null, but in that case, we should have a type for that to ensure uniformity and reduce duplication.

type SelectSingleReturnTypeOrNull = SelectSingleReturnType | null;
type SelectReturnTypeOrNull = SelectReturnType | null;

export function isNodeLike(value: SelectSingleReturnTypeOrNull): value is Node;
export function isArrayOfNodes(value: SelectReturnTypeOrNull): value is Node[];
export function isElement(value: SelectSingleReturnTypeOrNull): value is Element;

[cjbarth]

Would you prefer a new type of something like type Nullable<T> = T | null with Nullable<SelectReturnType>?

[JLRishe]

Yes, I think that would be good. Thanks.

[cjbarth]

Actually, it occurred to me that we can probably just take any and narrow from there.

[cjbarth]

I also wonder if we should more consistently name these checking function: e.g. should isDocumentFragment() be isDocumentFragmentNode() like we have isDocumentTypeNode()? That would keep it consistent and I think I'm the only consumer at this time, so I'd be happy to put up a PR to adjust this.

[JLRishe]

Yeah, I suppose it makes sense to have them all end with -Node. If you can make a PR with that targeting the types branch of this repo, I will merge it in.

[cjbarth]

make a PR with that targeting the types branch of this repo

See #125

[cjbarth]

It appears from #127 that people are using these new support functions. I've created #128 to continue to validate these functions as I work on the next release of xml-crypto and then node-saml and passport-saml after that. I'm working up the stack here. Feel free to leave #128 open until all that work is done. I can just use @ts-expect-error in the meantime.

[forty]

Note that TS gives a compile error even if you are not using the function, just because it's trying to typecheck all the types of the library (I'm in that case)

[forty]

I don't have a meaningful opinion for this specific use case but in my experience, it's often interesting to make typeguard slightly more clever / complex so that their input can be "unknown" (which is always the case if you are doing JS anyway), at the price of performance of course, as validation becomes more costly.

[cjbarth]

@forty , I can certainly make a change in my other branch to accommodate that suggestion. In fact, it does make sense to use unknown.

[cjbarth]

I've moved all this type checking code to another repo so that we don't muddy the purpose of xpath with them See https://github.com/xmldom/is-dom-node

I'll still gladly do what it takes to close out this PR or otherwise help it along as @JLRishe directs.