Adjust types to match exported functions

[cjbarth]

The selectWithResolver type was missing from the type definitions. As I was looking, I noticed a few others things might be wrong. I think this should cover everything. I'll try it in the xml-crypto project and report back when that migration is done. Until then, I'd like to get feedback.

[JLRishe]

Thank you for submitting this.

I'm not all that knowledgeable about .d.ts files and wasn't aware that they could contain functions. Would you mind explaining how one makes use of those functions?

One thing that I notice is that you are using instanceof Node and properties on Node in these functions, but to the best of my knowledge, the global Node object doesn't exist in the node.js environment, only in browsers. Does that mean these functions would fail if someone attempted to use them in node.js? Or does TypeScript take care of that somehow?

I would also think that those would fail to work on non-native nodes, such as those from @xmldom/xmldom, and this package is specifically designed to work with any standards-compliant DOM implementation.

Especially given that last part, I'm inclined to think that it would probably be wiser to use duck typing and a handful of node type constants rather than relying on the global Node object.

[cjbarth]

Indeed, duck typing does work better for this use-case. in xml-crypto, there are more libraries available, so we could get away with what you saw here. I've updated this code to reflect what is actually working in xml-crypto. Does this look better to you?

BTW, since the TS refactor of xml-crypto depends on this, this PR is a blocker to that getting released.

[JLRishe]

Thank you for this update. I've sent you a pull request with some proposed changes and also made some comments.

We should also have unit tests for the new functions you're adding. Can you include those in your PR?

[cjbarth]

I've made the changes you've discussed.

Side-note: I'd like to format your code, but I don't see a npm script to do so. What formatting tool do you use?

[JLRishe]

Thank you. I've requested a few more changes to the unit tests, and there's a re-activated comment about the return types in the XPathSelect interface.

Until now, I've been careful to leave the whitespace in xpath.js undisturbed so as to not have a huge diff in the history, but I was thinking that it's about time that I added a prettify process. I can do that once this PR is merged.

[cjbarth]

I've requested a few more changes to the unit tests

I'm sorry, I don't see these.

Otherwise, I've made all the changes that I think you've requested. Thanks for your review!

[cjbarth]

I added a prettify process

IIRC, eslint can format code and it is highly customizable, so you can just pick a style that most closely matches what you have now to minimize changes.

[cjbarth]

@JLRishe , I can't easily tell from reading the code; can select(expression: string, node: Node): SelectReturnType; also be select(expression: string, node: string): SelectReturnType;. I'm checking types in xml-crypto and I see that we are passing a string to select() and it seems to be working. I want to make sure the types are accurate here.

[JLRishe]

I'm sorry, I don't see these.

I'm referring to these comments:

can select(expression: string, node: Node): SelectReturnType; also be select(expression: string, node: string): SelectReturnType;

I don't see any way that would work.

I think what you're seeing is that xml-crypto has a select(node, xpath) export that calls off to the select(xpath, node) function from the xpath package (note the reversal in parameter order):

module.exports.xpath = function (node, xpath) {
  return select(xpath, node);
};

And xml-crypto's README.md and example.js are importing and calling that function that has the reversed parameters. Could you double check that and tell me if what you're referring to is somewhere else?

[cjbarth]

I'm referring to these comments:

Since they are "Pending", I don't see them. You have to complete the review for them to be visible to me :)

[JLRishe]

That's weird. I thought I sent them the same way I sent all the previous comments. Hopefully they're visible now.

[cjbarth]

Hopefully they're visible now.

They are. I'll resolve these as I push changes related to them. node-saml/xml-crypto#325 is now importing this PR at its HEAD so that I can make sure 100% of these changes are working in real code. I'll convert this to a Draft until it is ready to merge. Thanks again for your attention.

[cjbarth]

@JLRishe , one of the tests is failing, and it isn't because of any code I wrote. It appears that ProcessingInstruction doesn't have a nodeName property. Since it inherits from Node, and Node has a nodeName, this is a bug in this library.

    it('should correctly identify a Node of type ProcessingInstruction', () => {
            var doc = parseXml('<book />');
            var pi = doc.createProcessingInstruction('xml-stylesheet', 'href="mycss.css" type="text/css"');

            assert.ok(xpath.isNodeLike(pi));
            assert.ok(xpath.isProcessingInstruction(pi));
            assert.ok(!xpath.isComment(pi));
        });

Should have a nodeName of "xml-stylesheet", but it is currently undefined.

[JLRishe]

It appears that ProcessingInstruction doesn't have a nodeName property. Since it inherits from Node, and Node has a nodeName, this is a bug in this library.

The processing instruction in your test is being created by the @xmldom/xmldom package. The problem you're describing has nothing to do with this xpath package.

It does indeed appear that the DOM 3 standard indicates that processing instructions should have a nodeName property with the same value as its target property, so I would encourage filing an issue on their repo, but I imagine it would be a long time before they addressed it, and having these new functions work with that package is ideal, so please find a suitable workaround.

I think removing the nodeName check from the isNodeLike function would be ok. (typeof value.nodeName === "string" || typeof value.target === "string") would also be ok.

[cjbarth]

filing an issue on their repo

Done: xmldom/xmldom#505

[cjbarth]

I've put up a PR to get this fixed in xmldom: xmldom/xmldom#509

[cjbarth]

I've put up a PR to get this fixed in xmldom: xmldom/xmldom#509

This PR has landed and is schedule for v0.9.0 release. Once that release happens, we can revisit this PR to adjust the isNodeOfType block to remove the special exception for ProcessingInstruction.

[cjbarth]

This is now no longer needed since @xmldom/xmldom pushed a new release. I've reverted the work-arounds and updated the dependency.

FWIW, I note that engine in package.json specified a minimum version of 0.6. I tried installing that, and couldn't.

I installed 0.12 and the tests failed because that version of Node doesn't support const.

I installed v4 and that failed because it didn't support destructuring.

I installed v6 and that failed because yargs-parser didn't support anything less than v10.

I installed v10 and the tests passed.

All that to say, the package-lock.json was changed to "lockfileVersion": 1 from "lockfileVersion": 2 in order to support the older version of Node. I personally recommend doing all development on the oldest-supported version of Node to prevent overlooking these kinds of things.

[JLRishe]

The xpath.js file, which is the only code that this package exports, does not use const, nor destructuring, nor yargs-parser.

The tests use modern JavaScript, but the main file does not. I have been careful to ensure that the main code remains ES5 compliant (though I have just found that an incompatibility slipped in some time ago). I have just merged an ES5 validation script into the master branch.

Please revert the change to the "engines" setting, and the lock file format. I realize that not having a good way to test the main code against its stated node version is unideal, but that is not a discussion to have in this PR, which has gone far beyond its originally stated purpose.

[cjbarth]

Please revert the change to the "engines" setting, and the lock file format.

Done. I generated package-lock.json using Node@LTS. Since there are no dependencies, only devDependencies, this shouldn't be a problem for anyone. My apologies for creating extra work for you.

which has gone far beyond its originally stated purpose

Is there something you'd like to see broken out into multiple PRs?

[JLRishe]

Is there something you'd like to see broken out into multiple PRs?

I would have preferred to have the node type test functions in a separate PR. If this were just the .d.ts changes, it could have been merged a lot sooner. But it's ok. Thank you for sticking through this PR.

[cjbarth]

I'm not worried about the delay as the PR over at xml-crypto needs all of this for correct operation. Once this gets released then we can unblock that PR. I appreciate your sticking with me on this mess too; I know it was a lot.

[JLRishe]

The new version is now published to npm. Thanks!

[iahu]

that is strange to change the select return type to Array<Node> | SelectedValue, and this change break my codes.

I think the return type of select(expression: string, node: Node, single: false) should be Array<SelectedValue> | undefined | null, it should not return a single value type as result.

• if it selected somethings, (if there is just one selected "Value", the result should be transform to Array<SelectedValue>)
• if it selected nothing, return a empty array [];
• if it get a invalid selector, return null or undefined

BTW, if you make a breaking change, you should publish a MAJOR number version.

[JLRishe]

@iahu I am sorry that this update is causing trouble for you. In this update, the type definitions were modified to be consistent with the actual behavior of the package's implementation. The actual behavior of the package's exported functions has not changed. The type definitions were previously incorrect and have been corrected.

Per the XPath 1.0 spec, an XPath expression can evaluate to:

• A node set (0 or more nodes)
• A string
• A number
• A boolean

As you can see, it cannot evaluate to a set of strings, or to a null value. This package's select(_, _, false) function is consistent with this, so Array<SelectedValue> | undefined | null would not be correct. (Note: I think there is still a little room for correction in the type definitions since they currently imply that select(_, _, false) could return a single Node or null, which is not true.)

The author of this PR has added several utility functions that can help to converge the result of select() to the correct type, though it looks like there is currently an issue with the isArrayOfNodes function: #121

Personally, I think select() isn't very well suited for TypeScript, because of the ambiguity of its return type. For that reason, I strongly suggest using parsed expressions which allow evaluating XPaths without such ambiguity, though we do not yet have type definitions for those.

[iahu]

after read a bit of the source code. I agree that, the current type declare fit the select function's results.

as my above comment, I want get the facts straight. I suggested to change the select function's results to be "Array of SelectedValue", which like the querySelectorAll DOM API, and as the same time the select1 just like the querySelector API.

[cjbarth]

Note: I think there is still a little room for correction in the type definitions since they currently imply that select(_, _, false) could return a single Node or null, which is not true.

I'm reviewing this to make adjustments where needed. Are you hereby saying that selectWithResolver(_, _, _, false) will never return Node, null, or undefined? I think I can see that from the code, but I want to double-check. I'll put up a PR with the other adjustments you've requested so you can comment.