-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add back types for evaluate #121
base: master
Are you sure you want to change the base?
Conversation
Thank you for catching this. I agree that One question about the exporting of |
Running this in node the helpers in the dom lib aren't available, even if the types are through the use of 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 Further to this using 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. |
I see. Yes, I can see why I originally thought you were exporting the If so, shouldn't it be
with a |
Unless I'm mistaken, It appears that this is where Of course, we could be talking about the 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 Just to be thorough in this discussion, I suppose we could also be talking about the Context Line 4580 in 0617cef
Reference: https://developer.mozilla.org/en-US/docs/Web/API/Document/evaluate |
On line 4588, this is called on installDOM3XPathSupport(exports, new XPathParser()); And the Unlike 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 So my inclination would be to add |
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. |
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); |
@lukeggchapman You're probably not the only person using 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. |
We shouldn't have to work around it. I also think that we might be doing something similar in |
There really isn't a need to work around it. Both Aside from some obscure browser interop situation, I don't think there's any good reason to use Thoughts? |
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. |
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. |
I've moved all this type checking code to another repo so that we don't muddy the purpose of I'll still gladly do what it takes to close out this PR or otherwise help it along as @JLRishe directs. |
Is there anything I can do to help with this? |
#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:
Also @cjbarth the type guard for isArrayOfNodes seems wrong since
SelectedValue
can not be narrowed toNode[]
. Should the value argument be of typeSelectReturnType
instead?