From 92f41b71c5c638f43820ee42b29bdf59ddfc5594 Mon Sep 17 00:00:00 2001 From: eyelidlessness Date: Wed, 30 Oct 2024 15:14:55 -0700 Subject: [PATCH] XPathDOMAdapter &co refactor: prologue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As of the previous commit, I still have a few uncommitted changes in the `@getodk/xpath` package. This commit incorporates all but one remaining change. Most of this is just commentary. **SUBSTANTIVE CHANGE: eliminate unnecessary `tee` call** The last change to LocationPathEvaluation is a little bit of a perf optimization freebie. The call here to `tee` has not been necessary for some time, if it ever was. It’s highly likely it was introduced in early prototyping, while debugging some unexpected iterable/iterator behavior. The call is pure overhead, which I’ve known for some time but haven’t had a good excuse to land this quick win. However, this is probably the ideal time to take the quick win: it will give us a less biased baseline for comparing downstream (i.e. `@getodk/xforms-engine`) performance: 1. Before: evaluating XForms expressions against the current WHAT DOM backing store 2. After: evaluating those same expressions against the engine’s own DOM representation (which will be expanded to incorporate the functionality commited up to this point, and broadly to begin supporting external secondary instances). **WHAT’S NEXT: one last `@getodk/xpath` change** The next commit will set the stage for integration of the preceding changes into `@getodk/xforms-engine`, by restoring the export of `XFormsXPathEvaluator` (currently a temporary alias to the WHAT DOM equipped `DefaultXFormsXPathEvaluator`) so its breaking changes can be integrated. Once downstream integration is complete, we can finally dispatch `DefaultXFormsXPathEvaluator` to its test-only fate! --- .../xpath/src/adapter/WHAT/optimizations.ts | 27 +++++++++++++++++++ .../src/evaluations/LocationPathEvaluation.ts | 2 +- packages/xpath/src/functions/fn/node-set.ts | 1 + .../xpath/src/functions/javarosa/index.ts | 2 +- .../xpath/src/functions/javarosa/string.ts | 8 ++++++ 5 files changed, 38 insertions(+), 2 deletions(-) diff --git a/packages/xpath/src/adapter/WHAT/optimizations.ts b/packages/xpath/src/adapter/WHAT/optimizations.ts index c4149c8a2..c31992046 100644 --- a/packages/xpath/src/adapter/WHAT/optimizations.ts +++ b/packages/xpath/src/adapter/WHAT/optimizations.ts @@ -1,6 +1,9 @@ import type { WHATChildNode, WHATElement, WHATNode, WHATParentNode } from './WHATNode.ts'; import { getChildWHATElements, getContainingWHATDocument } from './traversal.ts'; +/** + * @todo optimization, but belongs in values.ts! + */ export const getQualifiedNamedWHATAttributeValue = ( node: WHATElement, namespaceURI: string | null, @@ -9,6 +12,9 @@ export const getQualifiedNamedWHATAttributeValue = ( return node.getAttributeNS(namespaceURI, localName); }; +/** + * @todo optimization, but belongs in values.ts! + */ export const getLocalNamedWHATAttributeValue = ( node: WHATElement, localName: string @@ -16,16 +22,25 @@ export const getLocalNamedWHATAttributeValue = ( return node.getAttribute(localName); }; +/** + * @todo optimization, but belongs in traversal.ts! + */ export const hasLocalNamedWHATAttribute = (node: WHATElement, localName: string): boolean => { return node.hasAttribute(localName); }; +/** + * @todo optimization, but belongs in traversal.ts! + */ export const getWHATElementByUniqueId = (node: WHATParentNode, id: string): WHATElement | null => { const element: Element | null = getContainingWHATDocument(node).getElementById(id); return element as WHATElement | null; }; +/** + * @todo optimization, but belongs in traversal.ts! + */ export const getWHATChildrenByLocalName = ( node: WHATParentNode, localName: string @@ -35,12 +50,18 @@ export const getWHATChildrenByLocalName = ( }); }; +/** + * @todo optimization, but belongs in traversal.ts! + */ export const getFirstWHATChildNode = (node: WHATNode): WHATChildNode | null => { const firstChild: ChildNode | null = node.firstChild; return firstChild as WHATChildNode | null; }; +/** + * @todo optimization, but belongs in traversal.ts! + */ export const getLastWHATChildNode = (node: WHATNode): WHATChildNode | null => { const lastChild: ChildNode | null = node.lastChild; @@ -53,12 +74,18 @@ type MaybeWHATElement = & WHATNode & Partial; +/** + * @todo optimization, but belongs in traversal.ts! + */ export const getFirstChildWHATElement = (node: WHATNode): WHATElement | null => { const firstElementChild: Element | null = (node as MaybeWHATElement).firstElementChild ?? null; return firstElementChild as WHATElement | null; }; +/** + * @todo optimization, but belongs in traversal.ts! + */ export const getLastChildWHATElement = (node: WHATNode): WHATElement | null => { const lastElementChild: Element | null = (node as MaybeWHATElement).lastElementChild ?? null; diff --git a/packages/xpath/src/evaluations/LocationPathEvaluation.ts b/packages/xpath/src/evaluations/LocationPathEvaluation.ts index 23d2adc0e..255618b58 100644 --- a/packages/xpath/src/evaluations/LocationPathEvaluation.ts +++ b/packages/xpath/src/evaluations/LocationPathEvaluation.ts @@ -659,7 +659,7 @@ export class LocationPathEvaluation this.rootNode = rootNode; this.timeZone = timeZone; - const [nodes] = tee(distinct(contextNodes)); + const nodes = distinct(contextNodes); this._nodes = nodes; diff --git a/packages/xpath/src/functions/fn/node-set.ts b/packages/xpath/src/functions/fn/node-set.ts index de12ab76b..f8cd5d495 100644 --- a/packages/xpath/src/functions/fn/node-set.ts +++ b/packages/xpath/src/functions/fn/node-set.ts @@ -86,6 +86,7 @@ export const localName = new StringFunction( return domProvider.getProcessingInstructionName(node); } + // TODO: double check expected behavior with namespace nodes return ''; } ); diff --git a/packages/xpath/src/functions/javarosa/index.ts b/packages/xpath/src/functions/javarosa/index.ts index 6dd7cf1c9..712780a8d 100644 --- a/packages/xpath/src/functions/javarosa/index.ts +++ b/packages/xpath/src/functions/javarosa/index.ts @@ -1,4 +1,4 @@ -import { JAVAROSA_NAMESPACE_URI } from '@getodk/common/constants/xmlns'; +import { JAVAROSA_NAMESPACE_URI } from '@getodk/common/constants/xmlns.ts'; import { FunctionLibrary } from '../../evaluator/functions/FunctionLibrary'; import * as string from './string.ts'; diff --git a/packages/xpath/src/functions/javarosa/string.ts b/packages/xpath/src/functions/javarosa/string.ts index 80d90fd91..7aa7ab6ee 100644 --- a/packages/xpath/src/functions/javarosa/string.ts +++ b/packages/xpath/src/functions/javarosa/string.ts @@ -1,6 +1,14 @@ +import { NodeSetFunction } from '../../evaluator/functions/NodeSetFunction.ts'; import { StringFunction } from '../../evaluator/functions/StringFunction.ts'; import { XFormsXPathEvaluator } from '../../xforms/XFormsXPathEvaluator.ts'; +/** + * @todo This could be a {@link NodeSetFunction}. If it were, that might be a + * good starting point for thinking about how we'll support: + * + * - `` in itext translations + * - `` (short, guidance, various media types) + */ export const itext = new StringFunction( 'itext', [{ arityType: 'required', typeHint: 'string' }],