Skip to content

Commit

Permalink
XPathDOMAdapter &co refactor: prologue
Browse files Browse the repository at this point in the history
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!
  • Loading branch information
eyelidlessness committed Oct 31, 2024
1 parent de08826 commit 92f41b7
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 2 deletions.
27 changes: 27 additions & 0 deletions packages/xpath/src/adapter/WHAT/optimizations.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand All @@ -9,23 +12,35 @@ export const getQualifiedNamedWHATAttributeValue = (
return node.getAttributeNS(namespaceURI, localName);
};

/**
* @todo optimization, but belongs in values.ts!
*/
export const getLocalNamedWHATAttributeValue = (
node: WHATElement,
localName: string
): string | null => {
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
Expand All @@ -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;

Expand All @@ -53,12 +74,18 @@ type MaybeWHATElement =
& WHATNode
& Partial<WHATElement>;

/**
* @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;

Expand Down
2 changes: 1 addition & 1 deletion packages/xpath/src/evaluations/LocationPathEvaluation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ export class LocationPathEvaluation<T extends XPathNode>
this.rootNode = rootNode;
this.timeZone = timeZone;

const [nodes] = tee(distinct(contextNodes));
const nodes = distinct(contextNodes);

this._nodes = nodes;

Expand Down
1 change: 1 addition & 0 deletions packages/xpath/src/functions/fn/node-set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export const localName = new StringFunction(
return domProvider.getProcessingInstructionName(node);
}

// TODO: double check expected behavior with namespace nodes
return '';
}
);
Expand Down
2 changes: 1 addition & 1 deletion packages/xpath/src/functions/javarosa/index.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand Down
8 changes: 8 additions & 0 deletions packages/xpath/src/functions/javarosa/string.ts
Original file line number Diff line number Diff line change
@@ -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:
*
* - `<output>` in itext translations
* - `<value form="...">` (short, guidance, various media types)
*/
export const itext = new StringFunction(
'itext',
[{ arityType: 'required', typeHint: 'string' }],
Expand Down

0 comments on commit 92f41b7

Please sign in to comment.