Add back types for evaluate

[lukeggchapman]

#116 removed the type definition for the evaluate function.

I've added it back in along with adding an export of XPathResult so that the following will work in Typescript:

import { DOMParser } from "@xmldom/xmldom";
import xpath from "xpath";

const xml = "<book><title>Harry Potter</title></book>";
const doc = new DOMParser().parseFromString(xml, "text/xml");
const node = xpath.evaluate(
  "/book/title",
  doc,
  null,
  xpath.XPathResult.STRING_TYPE,
  null
);

console.log("title", node.stringValue);

Also @cjbarth the type guard for isArrayOfNodes seems wrong since SelectedValue can not be narrowed to Node[]. Should the value argument be of type SelectReturnType instead?

[JLRishe]

Thank you for catching this.

I agree that SelectReturnType makes more sense as the parameter type for isArrayOfNodes, but hopefully @cjbarth can sign off on that as well.

One question about the exporting of XPathResult - I believe that is a built-in interface defined in the same lib.dom.d.ts where the Node interface is defined. So is there really a reason to export it? It seems like the only reason you're even able to reference it in the first place is that it's defined in the dom module, since it isn't defined anywhere in xpath.d.ts.

[lukeggchapman]

Running this in node the helpers in the dom lib aren't available, even if the types are through the use of /// <reference lib="dom" />.

import { DOMParser } from "@xmldom/xmldom";
import xpath from "xpath";

const xml = "<book><title>Harry Potter</title></book>";
const doc = new DOMParser().parseFromString(xml, "text/xml");
const node = xpath.evaluate(
  "/book/title",
  doc,
  null,
  XPathResult.STRING_TYPE,
  null
);

console.log("title", node.stringValue);

Fails at runtime with ReferenceError: XPathResult is not defined To be able to use the Polyfill that is created by xpath.js it needs to be exported in the type definitions.

Further to this using /// <reference lib-dom" /> in any module imports the dom lib for the entire project, and incorrectly masks errors when using them. Eg the above should have failed at compile time with Cannot find name 'XPathResult'.

Since @xmldom/xmldom does this as well, it's a larger issue than this PR, and will need some more thought to remove it from both packages.

[JLRishe]

I see. Yes, I can see why XPathResult.STRING_TYPE would fail at runtime. I hadn't noticed that line in your original comment and thought this was about the return type of evaluate.

I originally thought you were exporting the XPathResult type definition, but it seems what you're actually looking to do is export the xpath.XPathResult property, yes?

If so, shouldn't it be

export const XPathResult: XPathResult;

with a const instead of braces, and the type indicated? Or does export { XPathResult } achieve that same result? I suspect that the latter is exporting the XPathResult type from lib-dom as though it were a property of xpath, while the former indicates that xpath has an XPathResult property that implements the XPathResult interface. So the former might be a little better in terms of conveying intent, but please let me know what you think.

[cjbarth]

Unless I'm mistaken, xpath shouldn't have an evaluate() because it is a method on a Document instance. So, first you should get a Document instance, then, the type system would allow you to call evaluate() on that. I can certainly help with the definitions for that, but I was trying to make the definitions match reality as much as possible, and, for the life of me, I can't find where evaluate() is exported as part of xpath, so I didn't include it.

It appears that this is where xpath.evaluate() goes, which means that, if we are going to add types for evaluate(), we should also add types for createExpression() and createNSResolver(). But we also need to have a discussion around what the "current document" is in the case of a Node environment.

Of course, we could be talking about the evaluate() of the XPathEvaluator interface.

As I read through the documentation on this more, it feels like even the types that we currently have are potentially wrong. It seems we should adjust the types to only export parse() and the response type of parse() should be a type that includes these other types as per https://github.com/goto100/xpath/blob/master/docs/XPathEvaluator.md. That, however, doesn't agree very well with the MDN documentation.

Just to be thorough in this discussion, I suppose we could also be talking about the evaluate() method of the XPathExpression interface.

Context https://github.com/goto100/xpath/blob/0617ceffaa782f76c39df8478b5ab505edeb4e2f/xpath.js#L4580 Reference: https://developer.mozilla.org/en-US/docs/Web/API/Document/evaluate

[JLRishe]

On line 4588, this is called on exports:

    installDOM3XPathSupport(exports, new XPathParser());

And the exports function you indicated (which indirectly calls XPathExpression#evaluate) is attached to exports.

Unlike createExpression and createNSResolver, The ability to use xpath.evaluate() has been documented in this package's README for 7 years, and included in the .d.ts file ever since the .d.ts file was added 6 years ago.

It stands to reason that there are people whose code relies on this function and that is probably how @lukeggchapman found this issue so quickly.

I think I would need to see a compelling reason to explicitly advertise createExpression and createNSResolver as exported functions, and I suspect there isn't one. Those are primarily included as a polyfill for attaching to a DOM document.

So my inclination would be to add evaluate back to the .d.ts, and not the other two.

[lukeggchapman]

If so, shouldn't it be

export const XPathResult: XPathResult;

Happy to make this change as it's consistent with the rest of the definitions.

@JLRishe beat me to it, an implementation I had working before the weekend using what is in the README.md is no longer working. However I only started using it last week.

I'm able to workaround needing the createNSResolver, by just creating a shape that meets the type requirement:

const namespaces = {
  n: "some:namespace",
};

const namespaceResolver = {
  lookupNamespaceURI: (ns: string | null) =>
    ns && namespaces.hasOwnProperty(ns)
      ? namespaces[ns as keyof typeof namespaces]
      : null,
};

const result = xpath.evaluate(
  "/some/xpath",
  xmlDoc,
  namespaceResolver,
  xpath.XPathResult.UNORDERED_NODE_ITERATOR_TYPE,
  null
);

It's also possible for me to extend the module definition to add my own evaluate, but would prefer to just have it defined from the package to begin with.

[lukeggchapman]

That reminds me, I was also working around the need for XPathResult to be exported by just defining my own enum for the types, so if you're concerning about adding that I can leave it out.

import { DOMParser } from "@xmldom/xmldom";
import xpath from "xpath";

enum XmlTypesEnum {
  ANY_TYPE,
  NUMBER_TYPE,
  STRING_TYPE,
  BOOLEAN_TYPE,
  UNORDERED_NODE_ITERATOR_TYPE,
  ORDERED_NODE_ITERATOR_TYPE,
  UNORDERED_NODE_SNAPSHOT_TYPE,
  ORDERED_NODE_SNAPSHOT_TYPE,
  ANY_UNORDERED_NODE_TYPE,
  FIRST_ORDERED_NODE_TYPE,
}

const xml = "<book><title>Harry Potter</title></book>";
const doc = new DOMParser().parseFromString(xml, "text/xml");
const node = xpath.evaluate(
  "/book/title",
  doc,
  null,
  XmlTypesEnum.STRING_TYPE,
  null
);

console.log("title", node.stringValue);

[JLRishe]

@lukeggchapman You're probably not the only person using evaluate so I think we should add it back.

However, you may also want to consider using a parsed expression (though this API unfortunately hasn't been added to the .d.ts yet). It offers an unambiguous API and performance improvements since you can store parsed XPaths in variables to avoid parsing them multiple times.

Your two most recent examples would become:

const namespaces = {
  n: "some:namespace",
};

const expression = xpath.parse('/some/xpath');

// unambiguously returns Array<Node>
const result = expression.select({ node: doc, namespaces });

and

const expression = xpath.parse('/book/title');

// title is a string
const title = expression.evaluateString({ node: doc });

console.log("title", title);

Given some time, I could add a .d.ts for this, but next week is probably the soonest I could do that.

[cjbarth]

I'm able to workaround needing the createNSResolver, by just creating a shape that meets the type requirement:

We shouldn't have to work around it. I also think that we might be doing something similar in xml-crypto.

[JLRishe]

We shouldn't have to work around it.

There really isn't a need to work around it. Both xpath.useNamespaces() and the evaluator on parsed expressions accept a Record<string, string> as a namespace map. The latter also accepts a (prefix: string) => string as a namespace map if there is any need for computation.

Aside from some obscure browser interop situation, I don't think there's any good reason to use xpath.evaluate, and I think there may be no reason to use selectWithResolver over the other options available. So I think I would go so far as to say the use of both of those should be discouraged and possibly that selectWithResolver shouldn't even be included in the .d.ts given that it wasn't there until now. The fact it's exported in the first place was probably just laziness on the part of whoever originally added the wrapper with the .select() functions.

Thoughts?

[cjbarth]

Aside from some obscure browser interop situation, I don't think there's any good reason to use xpath.evaluate

So there would be no need to add that type back in?

[JLRishe]

So there would be no need to add that type back in?

No, as I've said, it has already been in the .d.ts for 6 years, which means removing it is a breaking change, so it should be added back for backward compatibility. selectWithResolver, on the other hand, was just added last week, and I think it should be removed. I will work on getting xpath.parse and its related definitions added so that typescript users have the ability to use those.

[cjbarth]

selectWithResolver, on the other hand, was just added last week, and I think it should be removed

Generally speaking, if it exported, it should be typed. If the type should be removed, it should be refactored so as to not be exported. Otherwise, you run into the problem that got me working on this in the first place: trying to use TypeScript and finding that something that works in JavaScript is broken in TypeScript because the type checking in wrong. So, users either need to mark that as an allowable error in their TypeScript compiler, augment the provided types, or they need to come here and try to get them added.

One interesting thing about working with TypeScript over JavaScript is that problems like this (exposed API) are made much more apparent.

[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.

[cjbarth]

Is there anything I can do to help with this?