fix: get rid of any types and allow even deeper ast traversal

even though it's a method more, the context node.js has to carry over is less, allowing up to 700ish depth.
vendure-ecommerce · Nov 25, 2023 · cd8bfbf · cd8bfbf
1 parent 2d91022
commit cd8bfbf
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 37 deletions.
diff --git a/src/parsers/pipe.parser.ts b/src/parsers/pipe.parser.ts
@@ -12,7 +12,10 @@ import {
 	Interpolation,
 	Call,
 	TmplAstIfBlock,
-	TmplAstSwitchBlock
+	TmplAstSwitchBlock,
+	TmplAstDeferredBlock,
+	TmplAstForLoopBlock,
+	TmplAstElement
 } from '@angular/compiler';
 
 import { ParserInterface } from './parser.interface.js';
@@ -21,47 +24,56 @@ import { isPathAngularComponent, extractComponentInlineTemplate } from '../utils
 
 export const TRANSLATE_PIPE_NAMES = ['translate', 'marker'];
 
-/**
- * this method is meant to only cause 1 call stack per ast depth, to prevent to run into call stack limits on large templates
- */
-function traverseAst<RESULT extends unknown[]>(node: any, visitor: (node: TmplAstNode) => RESULT, result: RESULT) {
-	if (!node) {
-		return;
-	}
-
-	if (Array.isArray(node)) {
-		for (const child of node) {
-			traverseAst(child, visitor, result);
+function traverseAstNodes<RESULT extends unknown, NODE extends TmplAstNode | TmplAstElement>(
+	nodes: (NODE | null)[],
+	visitor: (node: NODE) => RESULT[],
+	accumulator: RESULT[] = []
+): RESULT[] {
+	for (const node of nodes) {
+		if (node) {
+			traverseAstNode(node, visitor, accumulator);
 		}
-		return;
 	}
 
-	result.push(...visitor(node));
+	return accumulator;
+}
 
-	const children = [];
-	children.push(node.children ?? []);
+function traverseAstNode<RESULT extends unknown, NODE extends TmplAstNode | TmplAstElement>(
+	node: NODE,
+	visitor: (node: NODE) => RESULT[],
+	accumulator: RESULT[] = []
+): RESULT[] {
+	accumulator.push(...visitor(node));
+
+	const children: TmplAstNode[] = [];
+	// children of templates, html elements or blocks
+	if ('children' in node && node.children) {
+		children.push(...node.children);
+	}
 
 	// @for blocks
-	children.push(node.empty ?? []);
+	if (node instanceof TmplAstForLoopBlock) {
+		children.push(node.empty);
+	}
 
 	// @deferred blocks
-	children.push(node.error ?? []);
-	children.push(node.loading ?? []);
-	children.push(node.placeholder ?? []);
+	if (node instanceof TmplAstDeferredBlock) {
+		children.push(node.error);
+		children.push(node.loading);
+		children.push(node.placeholder);
+	}
 
 	// @if block
 	if (node instanceof TmplAstIfBlock) {
-		children.push(node.branches.map((inner) => inner.children).flat() ?? []);
+		children.push(...node.branches.flatMap((inner) => inner.children));
 	}
 
 	// @switch block
 	if (node instanceof TmplAstSwitchBlock) {
-		children.push(node.cases.map((inner) => inner.children).flat() ?? []);
+		children.push(...node.cases.flatMap((inner) => inner.children));
 	}
 
-	for (const child of children.flat()) {
-		traverseAst(child, visitor, result);
-	}
+	return traverseAstNodes(children, visitor, accumulator);
 }
 
 export class PipeParser implements ParserInterface {
@@ -73,8 +85,8 @@ export class PipeParser implements ParserInterface {
 		let collection: TranslationCollection = new TranslationCollection();
 		const nodes: TmplAstNode[] = this.parseTemplate(source, filePath);
 
-		const pipes: BindingPipe[] = [];
-		traverseAst(nodes, (node) => this.findPipesInNode(node), pipes);
+		const pipes = traverseAstNodes(nodes, (node) => this.findPipesInNode(node));
+
 		pipes.forEach((pipe) => {
 			this.parseTranslationKeysFromPipe(pipe).forEach((key: string) => {
 				collection = collection.add(key, '', filePath);

diff --git a/tests/parsers/pipe.parser.spec.ts b/tests/parsers/pipe.parser.spec.ts
@@ -370,20 +370,16 @@ describe('PipeParser', () => {
 			]);
 		});
 
-		it('should handle deep ast without hitting the call stack limit', () => {
+		it('should handle ast with arbitrary depth without hitting the call stack limit', () => {
+			const depth = 500;
 			const contents = `
-				<i><i><i><i><i><i><i><i><i><i><i><i>
-					<i><i><i><i><i><i><i><i><i><i><i><i>
-						<i><i><i><i><i><i><i><i><i><i><i><i>
-							<i><i><i><i><i><i><i><i><i><i><i><i>
-								{{ 'deep' | translate }}
-							</i></i></i></i></i></i></i></i></i></i></i></i>
-						</i></i></i></i></i></i></i></i></i></i></i></i>
-					</i></i></i></i></i></i></i></i></i></i></i></i>
-				</i></i></i></i></i></i></i></i></i></i></i></i>
+				${Array(depth).fill('<i>').join('')}
+					{{ 'deep' | translate }}
+				${Array(depth).fill('</i>').join('')}
 			`;
 
 			const keys = parser.extract(contents, templateFilename)?.keys();
+			expect(contents).to.contain('<i><i><i><i><i><i>');
 			expect(keys).to.deep.equal(['deep']);
 		});
 	});