Clean up some types and rename functions for clarity

[cjbarth]

There are some types that aren't correct. Adjust these types to be more accurate and rename some type-narrowing functions to have a more clear name.

This branch is in use by xml-crypto to validate that this methodology works.

[cjbarth]

Per a comment from @forty, incoming types will be adjusted to unknown and the JS made more resilient.

[cjbarth]

@forty, how does this look to you? I still have to test it in xml-crypto, but I'm interested in your thoughts.

[forty]

That is what I had in mind yes 👍 With that said, I had a look to the full Node interface in nodejs type definition, and now I'm a bit torn, because the interface is huge so we cannot really validate it all, and the type guard could cause some mistake if someone use it with an object which is not a proper Node but is close enough that it can pass the type guard check.

Practically, I think this is fine, unless someone is really misusing the library anyway, and personally, I'd find your current solution good enough to include in my own codebase (I'm just highlighting the caveats)

[forty]

One semi-elegant and somewhat safer way of checking your types without the helper is to use the class provided by you dom implementation, when available (not really the case for xmldom unfortunately)

const xml = require('@xmldom/xmldom');
const xpath = require('xpath');
const doc = new xml.DOMParser().parseFromString('<doc><elem /></doc>');

const sel = xpath.select('//elem', doc);

// sadly not exposed to us
const dom = require('@xmldom/xmldom/lib/dom');

console.log(sel[0] instanceof dom.Element);
console.log(sel[0] instanceof dom.Node);

Maybe this tells us those helpers belong to xmldom instead. Not really sure.

[cjbarth]

the interface is huge so we cannot really validate it all, and the type guard could cause some mistake if someone use it with an object which is not a proper Node but is close enough that it can pass the type guard check

This is a challenge that I ran into as well. I actually had a few more properties that I was checking, but found that they weren't included in these synthetic Node objects that are being generated. I figured that with these properties the confidence interval would be pretty high unless they were trying to cause a problem. If something was this close to a Node, but not a Node, they should know about it before ever using these functions.

Maybe this tells us those helpers belong to xmldom instead

Let's reach out to a maintainer at xmldom to see what their thoughts are. @karfau, what do you think? Should a set of checks like this be included in the xmldom library instead of here? I'd be happy to make a PR with the relevant functions over there.

[karfau]

@cjbarth Are you talking about the type guards that you implemented in this PR?

I think for xmldom it could make sense to provide type guards based on the prototypes that are currently only exported from the internal modules, not on certain property types or values. At least that would be my first thoughts when considering it.
(The main reason for not exporting them is to avoid people using the constructor functions directly instead of the DOM API. I'm not aware of a clean way to expose those "interfaces" without exposing the constructors when using javascript.)

Regarding using xmldom as a ponyfill and only when the native API is not available (do I remember correctly that this is what xpath is doing?), the check based on the prototype would of course not work, so maybe it would make sense to do it the other way...

The types provided by xmldom also refer to dom, but this is actually just a short cut to not rewrite the whole API in types. And it's not ideal because there are some differences.
Would love to generate the types from the jsdoc comments at some point.

The other aspect for xmldom is that I'm trying really hard to limit the API of xmldom to things that are defined in one of the related specs.
And so far I'm not aware of anything like that in those specs.

And from the perspective of a library that expects certain functions and properties to be present for something to be considered a Node, Element, ..., I think it makes perfect sense to contain those type guards.

This is my current perspective, based on the information I was able to extract from this thread and diff.
But I'm open for arguments :)

[cjbarth]

I'm trying really hard to limit the API of xmldom to things that are defined in one of the related specs.

This comment alone seems to eliminate the possibility of including such type guards in xmldom. Do you think it would be appropriate to have an @xmldom/xmldom-typeguards project or similar? Since we aren't dealing with a typical prototype chain, there really is a need to be able to have a reasonable degree of confidence that the object that is being interacted with is a certain type of object, and the only way to do so at this time, without getting crazy as @forty pointed out, would be to use code like identified in this PR to check isNodeLike and the various Node subtypes.

I originally put these guards in xpath because that library was doing the select() and there really is a need to be able to tell what came back from the call to select() and its relatives. Since there was already some inclination to have guards in this project, I just expanded support. If there is a desire by @JLRishe , or a feeling from @forty or @karfau that these checks should be elsewhere, I'm up for that too. I don't know that the @node-saml organization is the right place for them though since that is very non-obvious.

[karfau]

I actually had that thought while writing my previous message, but I don't have any capacity beyond the xmldom project itself right now.

I'm not aware whether it's possible to limit publishing rights to a single package in an npm org, but if that's possible I'm up for adding people to the org and making this possible, if it makes sense for enough people and there are enough volunteers to create and maintain it.
(Update: from the docs it looks like it is possible.)

And just to make it explicit: we are agreeing that these checks/guards should be working with the browser native implementation as well as with the xmldom implmentation, right?

[cjbarth]

I would love to hear from @JLRishe : Would you like to see all this extra checking code be moved to a dependent project over at @xmldom or would you like to keep it here? I'm up for whatever. I just really appreciate @forty 's comments and I want to do what is best for long-term maintenance and what the community might get the most use of. I can imagine that people might not want to take on a dependency of xpath or xmldom just to get these type-checks.

we are agreeing that these checks/guards should be working with the browser native implementation as well as with the xmldom implmentation

That would be a simple addition to the code I've already written.

In any case, I'd just write it in native TypeScript so that we didn't have to deal with definition files. If we all agree @karfau , I'd gladly use a bootstrapped blank project with all your typically tooling and integrate with that. Otherwise, I'd probably use the same type of pattern I've been using in node-saml and friends.

[karfau]

Since xmldom is not a typescript project so far, I'm fine with providing an empty repo and access rights, and the people maintaining it should create the setup they need to work with it.

The tooling in node-saml looks reasonable to me at first glance. Should I ever need to touch it I think I should be able to get started quickly.

[cjbarth]

Since we haven't heard from @JLRishe , and he is admittedly very busy, I'm in favor of doing as @karfau and @forty have suggested and get a project up that is just for this kind of type checking. xpath still needs a little internal type-checking, so we can leave the very basics of the code there and remove the exposed API in the next major release (I can make a PR for that).

In any case, I'm blocked here and will probably move forward with using a fork of this project so that I can get node-saml and passport-saml unblocked. I'd really like to not ship with a fork, but we also have people waiting on getting some features released for node-saml which we are holding up. I look forward to hearing @JLRishe 's thoughs, or getting that repo set up from @karfau. (If you set up the repo, will you also be doing the publishing of the package? If so, I'll not include my usual release-it scaffolding. Otherwise, I'll need rights to publish in the @xmldom namespace, at least for this new package.)

Adding @markstos due to his involvement with node-saml.

[karfau]

@cjbarth Sadly I don't have admin rights on npm yet for that scope. I'm trying to get them, but didn't get a response yet since my last comment.

What name would you want to have for the repo/package?
is-dom-node or just is-dom?

I would prefer to not be involved in publishing, but if it is the only way it can currently be done I'm fine with configuring my secret for an automated release.

[cjbarth]

I would imagine that is-dom-node would be more accurate. Even if we can't publish at this stage, we can npm install from a GitHub release. I might therefore include my release-it scaffolding unless you have a preferred way of preparing releases (I'm all ears).

[karfau]

@cjbarth xmldom/is-dom-node#1