Skip to content
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

Jump to bottom

Address issues with typings #123

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Address issues with typings #123

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e22c60a
Address issues with typings
JLRishe Jul 17, 2023
219546e
Have typings for single selects indicate they can return undefined
JLRishe Jul 17, 2023
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
28 changes: 14 additions & 14 deletions xpath.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
/// <reference lib="dom" />

export type SelectedValue = Node | string | number | boolean | null;
export type ScalarValue = string | number | boolean;

export type SelectReturnType = Array<Node> | SelectedValue;
export type SelectSingleReturnType = SelectedValue;
export type SelectReturnType = Array<Node> | ScalarValue;
export type SelectSingleReturnType = ScalarValue | Node | undefined;

export interface XPathSelect {
(expression: string, node: Node): SelectReturnType;
Expand Down Expand Up @@ -38,14 +38,14 @@ export function selectWithResolver(expression: string, node: Node, resolver: XPa
export function useNamespaces(namespaceMap: Record<string, string>): XPathSelect;

// Type guards to narrow down the type of the selected type of a returned Node object
export function isNodeLike(value: SelectedValue): value is Node;
export function isArrayOfNodes(value: SelectedValue): value is Node[];
export function isElement(value: SelectedValue): value is Element;
export function isAttribute(value: SelectedValue): value is Attr;
export function isTextNode(value: SelectedValue): value is Text;
export function isCDATASection(value: SelectedValue): value is CDATASection;
export function isProcessingInstruction(value: SelectedValue): value is ProcessingInstruction;
export function isComment(value: SelectedValue): value is Comment;
export function isDocumentNode(value: SelectedValue): value is Document;
export function isDocumentTypeNode(value: SelectedValue): value is DocumentType;
export function isDocumentFragment(value: SelectedValue): value is DocumentFragment;
export function isNodeLike(value: SelectSingleReturnType): value is Node;
export function isArrayOfNodes(value: SelectReturnType): value is Node[];
export function isElement(value: SelectSingleReturnType): value is Element;
export function isAttribute(value: SelectSingleReturnType): value is Attr;
export function isTextNode(value: SelectSingleReturnType): value is Text;
export function isCDATASection(value: SelectSingleReturnType): value is CDATASection;
export function isProcessingInstruction(value: SelectSingleReturnType): value is ProcessingInstruction;
export function isComment(value: SelectSingleReturnType): value is Comment;
export function isDocumentNode(value: SelectSingleReturnType): value is Document;
export function isDocumentTypeNode(value: SelectSingleReturnType): value is DocumentType;
export function isDocumentFragment(value: SelectSingleReturnType): value is DocumentFragment;
Comment on lines +41 to +51
Copy link

Choose a reason for hiding this comment

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

In testing this in xml-crypto it seems that it would be helpful to take in null and undefined as arguments, this way I don't have to have multiple checks.

Suggested change
export function isNodeLike(value: SelectSingleReturnType): value is Node;
export function isArrayOfNodes(value: SelectReturnType): value is Node[];
export function isElement(value: SelectSingleReturnType): value is Element;
export function isAttribute(value: SelectSingleReturnType): value is Attr;
export function isTextNode(value: SelectSingleReturnType): value is Text;
export function isCDATASection(value: SelectSingleReturnType): value is CDATASection;
export function isProcessingInstruction(value: SelectSingleReturnType): value is ProcessingInstruction;
export function isComment(value: SelectSingleReturnType): value is Comment;
export function isDocumentNode(value: SelectSingleReturnType): value is Document;
export function isDocumentTypeNode(value: SelectSingleReturnType): value is DocumentType;
export function isDocumentFragment(value: SelectSingleReturnType): value is DocumentFragment;
export function isNodeLike(value?: SelectSingleReturnType | null): value is Node;
export function isArrayOfNodes(value?: SelectReturnType | null): value is Node[];
export function isElement(value?: SelectSingleReturnType | null): value is Element;
export function isAttribute(value?: SelectSingleReturnType | null): value is Attr;
export function isTextNode(value?: SelectSingleReturnType | null): value is Text;
export function isCDATASection(value?: SelectSingleReturnType | null): value is CDATASection;
export function isProcessingInstruction(value?: SelectSingleReturnType | null): value is ProcessingInstruction;
export function isComment(value?: SelectSingleReturnType | null): value is Comment;
export function isDocumentNode(value?: SelectSingleReturnType | null): value is Document;
export function isDocumentTypeNode(value?: SelectSingleReturnType | null): value is DocumentType;
export function isDocumentFragment(value?: SelectSingleReturnType | null): value is DocumentFragment;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The definition of SelectSingleReturnType already includes undefined, so it doesn't seem like there's a need to re-indicate that in the function signatures. SelectReturnType doesn't include undefined, but I'm not sure I can see a good case for having isArrayOfNodes accept undefined.
And ? is for indicating a parameter that can be omitted completely, so that wouldn't be appropriate here.

I think I can see the value of having these function accept null, but in that case, we should have a type for that to ensure uniformity and reduce duplication.

type SelectSingleReturnTypeOrNull = SelectSingleReturnType | null;
type SelectReturnTypeOrNull = SelectReturnType | null;

export function isNodeLike(value: SelectSingleReturnTypeOrNull): value is Node;
export function isArrayOfNodes(value: SelectReturnTypeOrNull): value is Node[];
export function isElement(value: SelectSingleReturnTypeOrNull): value is Element;

Copy link

Choose a reason for hiding this comment

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

Would you prefer a new type of something like type Nullable<T> = T | null with Nullable<SelectReturnType>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think that would be good. Thanks.

Copy link

Choose a reason for hiding this comment

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

Actually, it occurred to me that we can probably just take any and narrow from there.