Skip to content

fix: restore more Node and ProcessingInstruction types #726

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Sep 14, 2024
Merged

fix: restore more Node and ProcessingInstruction types #726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d23c007
fix: restore Node and ProcessingInstruction prop types
karfau Sep 9, 2024
b92962c
chore: resolve CodeQL warnings
karfau Sep 9, 2024
3e161a8
docs(types): update comments after review
karfau Sep 14, 2024
23d0273
fix: ProcessingInstruction extends CharacterData
karfau Sep 14, 2024
730b14d
Merge remote-tracking branch 'origin/master' into missing-node-pi-types
karfau Sep 14, 2024
0db88a2
fix: unify checks in hasInsertableNodeType
karfau Sep 14, 2024
9173375
docs: assert actual values for ProcessingInstruction
karfau Sep 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions examples/typescript-node-es6/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
Text,
XMLSerializer,
Element,
ProcessingInstruction,
} from '@xmldom/xmldom';

const failedAssertions: Error[] = [];
Expand Down Expand Up @@ -75,6 +76,14 @@ new ParseError('message', {}, domException);
assert(Node.ATTRIBUTE_NODE, 2);
assert(Node.DOCUMENT_POSITION_CONTAINS, 8);

// there are no real Node instances,
// but we want to check that the Node type provides these props
const fakeNode = {} as unknown as Node;
assert(fakeNode.nodeType, undefined);
assert(fakeNode.lineNumber, undefined);
assert(fakeNode.columnNumber, undefined);
assert(fakeNode.textContent, undefined);
Copy link
Collaborator

@Ponynjaa Ponynjaa Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't really make sense since no matter what property you try to access it's always gonna be undefined since fakeNode is just an empty object. You should probably just use an Element or something else that extends Node.

Copy link
Member Author

@karfau karfau Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the comment above this section, yes, there is no actual instance of type Node, but these assertions should make sure that those properties are available on the type Node, not only on some sub type/class.
When using assert, the CodeQL checks will complain that the values are unused.
And yes, I'm asserting that the value is undefined, which only makes sense in the context of this assertion.

Copy link
Collaborator

@Ponynjaa Ponynjaa Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I think I get it now... so basically if you would try to do this: assert(fakeNode.doesntExist, undefined) JS would be fine with it but since the TS compiler checks this the check would fail correctly.


assert(new NodeList().length, 0);

const impl = new DOMImplementation();
Expand All @@ -101,6 +110,15 @@ assert(element.nodeType, Node.ELEMENT_NODE);
assert(element.ownerDocument, doc1);
assert(element.attributes instanceof NamedNodeMap, true);

const pi = doc1.createProcessingInstruction('target', 'data');
assert(pi.nodeType, Node.PROCESSING_INSTRUCTION_NODE);
assert(pi.target, 'target');
assert(pi.data, 'data');
assert(pi.target, pi.nodeName);
assert(pi.data, pi.nodeValue);
assert(pi instanceof ProcessingInstruction, true);
assert(pi instanceof CharacterData, true);

const cdata = doc1.createCDATASection('< &');
assert(cdata instanceof CharacterData, true);
assert(cdata instanceof CDATASection, true);
Expand Down
43 changes: 34 additions & 9 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ declare module '@xmldom/xmldom' {
* cannot have children will throw an exception.
*
* **This behavior is slightly different from the in the specs**:
* - undeclared properties: nodeType, baseURI, isConnected, parentElement, textContent
* - undeclared properties: baseURI, isConnected, parentElement
* - missing methods: contains, getRootNode, isEqualNode, isSameNode
*
* @see http://www.w3.org/TR/2000/REC-DOM-Level-2-Core-20001113/core.html#ID-1950641247
Expand Down Expand Up @@ -396,6 +396,12 @@ declare module '@xmldom/xmldom' {
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Node/nodeName)
*/
readonly nodeName: string;
/**
* Returns the type of node.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Node/nodeType)
*/
readonly nodeType: number;
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Node/nodeValue) */
nodeValue: string | null;
/**
Expand All @@ -420,6 +426,19 @@ declare module '@xmldom/xmldom' {
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/Node/previousSibling)
*/
readonly previousSibling: Node | null;
/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Node/textContent) */
textContent: string | null;

/**
* Zero based line position inside the parsed source,
* if the `locator` was not disabled.
*/
lineNumber?: number;
/**
* One based column position inside the parsed source,
* if the `locator` was not disabled.
*/
columnNumber?: number;

/** [MDN Reference](https://developer.mozilla.org/docs/Web/API/Node/appendChild) */
appendChild(node: Node): Node;
Expand Down Expand Up @@ -956,12 +975,7 @@ declare module '@xmldom/xmldom' {
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/CharacterData)
*/
var CharacterData: {
// instanceof pre ts 5.3
(val: unknown): val is CharacterData;
// instanceof post ts 5.3
[Symbol.hasInstance](val: unknown): val is CharacterData;
};
var CharacterData: InstanceOf<CharacterData>;

/**
* The textual content of Element or Attr. If an element has no markup within its content, it has
Expand Down Expand Up @@ -1059,8 +1073,20 @@ declare module '@xmldom/xmldom' {
}
var Notation: InstanceOf<Notation>;

interface ProcessingInstruction extends Node {
interface ProcessingInstruction extends CharacterData {
nodeType: typeof Node.PROCESSING_INSTRUCTION_NODE;
/**
* A string representing the textual data contained in this object.
* For `ProcessingInstruction`, that means everything that goes after the `target`, excluding
* `?>`.
*
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/CharacterData/data)
*/
data: string;
/**
* A string containing the name of the application.
* [MDN Reference](https://developer.mozilla.org/docs/Web/API/ProcessingInstruction/target) */
readonly target: string;
}
var ProcessingInstruction: InstanceOf<ProcessingInstruction>;

Expand Down Expand Up @@ -1368,7 +1394,6 @@ declare module '@xmldom/xmldom' {
class XMLSerializer {
serializeToString(node: Node, nodeFilter?: (node: Node) => boolean): string;
}

// END ./lib/dom.js

// START ./lib/dom-parser.js
Expand Down
15 changes: 8 additions & 7 deletions lib/dom.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -979,7 +979,7 @@ DOMImplementation.prototype = {
* cannot have children will throw an exception.
*
* **This behavior is slightly different from the in the specs**:
* - undeclared properties: nodeType, baseURI, isConnected, parentElement, textContent
* - undeclared properties: baseURI, isConnected, parentElement
* - missing methods: contains, getRootNode, isEqualNode, isSameNode
*
* @class
Expand Down Expand Up @@ -1587,12 +1587,13 @@ function hasValidParentNodeType(node) {
function hasInsertableNodeType(node) {
return (
node &&
(isElementNode(node) ||
node instanceof CharacterData ||
isDocTypeNode(node) ||
node.nodeType === Node.DOCUMENT_FRAGMENT_NODE ||
(node.nodeType === Node.CDATA_SECTION_NODE ||
node.nodeType === Node.COMMENT_NODE ||
node.nodeType === Node.PROCESSING_INSTRUCTION_NODE)
node.nodeType === Node.DOCUMENT_FRAGMENT_NODE ||
node.nodeType === Node.DOCUMENT_TYPE_NODE ||
node.nodeType === Node.ELEMENT_NODE ||
node.nodeType === Node.PROCESSING_INSTRUCTION_NODE ||
node.nodeType === Node.TEXT_NODE)
);
}

Expand Down Expand Up @@ -2481,7 +2482,7 @@ function ProcessingInstruction(symbol) {
checkSymbol(symbol);
}
ProcessingInstruction.prototype.nodeType = PROCESSING_INSTRUCTION_NODE;
_extends(ProcessingInstruction, Node);
_extends(ProcessingInstruction, CharacterData);
function XMLSerializer() {}
XMLSerializer.prototype.serializeToString = function (node, nodeFilter) {
return nodeSerializeToString.call(node, nodeFilter);
Expand Down