Remove optimization from $("node").item case

packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/__tests__/built-ins-parser.test.ts

@@ -56,9 +56,12 @@ describe('BuiltInsParser', () => {
 
 	describe('$(...)', () => {
 		const cases: Array<[string, BuiltInsParserState]> = [
-			['$("nodeName")', new BuiltInsParserState({ neededNodeNames: new Set(['nodeName']) })],
 			[
-				'$("nodeName"); $("secondNode")',
+				'$("nodeName").first()',
+				new BuiltInsParserState({ neededNodeNames: new Set(['nodeName']) }),
+			],
+			[
+				'$("nodeName").all(); $("secondNode").matchingItem()',
 				new BuiltInsParserState({ neededNodeNames: new Set(['nodeName', 'secondNode']) }),
 			],
 		];
@@ -90,6 +93,29 @@ describe('BuiltInsParser', () => {
 			const state = parseAndExpectOk(code);
 			expect(state).toEqual(new BuiltInsParserState());
 		});
+
+		test.each([
+			'$("node").item',
+			'$("node")["item"]',
+			'$("node")[variable]',
+			'var a = $("node")',
+			'let a = $("node")',
+			'const a = $("node")',
+			'a = $("node")',
+		])('should require all nodes if %s is used', (code) => {
+			const state = parseAndExpectOk(code);
+			expect(state).toEqual(new BuiltInsParserState({ needsAllNodes: true }));
+		});
+
+		test.each(['$("node").first()', '$("node").last()', '$("node").all()', '$("node").params'])(
+			'should require only accessed node if %s is used',
+			(code) => {
+				const state = parseAndExpectOk(code);
+				expect(state).toEqual(
+					new BuiltInsParserState({ needsAllNodes: false, neededNodeNames: new Set(['node']) }),
+				);
+			},
+		);
 	});
 
 	describe('ECMAScript syntax', () => {

packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/acorn-helpers.ts

@@ -0,0 +1,28 @@
+import type {
+	AssignmentExpression,
+	Identifier,
+	Literal,
+	MemberExpression,
+	Node,
+	VariableDeclarator,
+} from 'acorn';
+
+export function isLiteral(node?: Node): node is Literal {
+	return node?.type === 'Literal';
+}
+
+export function isIdentifier(node?: Node): node is Identifier {
+	return node?.type === 'Identifier';
+}
+
+export function isMemberExpression(node?: Node): node is MemberExpression {
+	return node?.type === 'MemberExpression';
+}
+
+export function isVariableDeclarator(node?: Node): node is VariableDeclarator {
+	return node?.type === 'VariableDeclarator';
+}
+
+export function isAssignmentExpression(node?: Node): node is AssignmentExpression {
+	return node?.type === 'AssignmentExpression';
+}

packages/@n8n/task-runner/src/js-task-runner/built-ins-parser/built-ins-parser.ts

@@ -1,9 +1,16 @@
-import type { Program } from 'acorn';
+import type { CallExpression, Identifier, Node, Program } from 'acorn';
 import { parse } from 'acorn';
-import { simple } from 'acorn-walk';
+import { ancestor } from 'acorn-walk';
 import type { Result } from 'n8n-workflow';
 import { toResult } from 'n8n-workflow';
 
+import {
+	isAssignmentExpression,
+	isIdentifier,
+	isLiteral,
+	isMemberExpression,
+	isVariableDeclarator,
+} from './acorn-helpers';
 import { BuiltInsParserState } from './built-ins-parser-state';
 
 /**
@@ -26,47 +33,103 @@ export class BuiltInsParser {
 	private identifyBuiltInsByWalkingAst(ast: Program) {
 		const accessedBuiltIns = new BuiltInsParserState();
 
-		simple(ast, {
-			CallExpression(node) {
-				// $(...)
-				const isDollar = node.callee.type === 'Identifier' && node.callee.name === '$';
-				if (!isDollar) return;
+		ancestor(
+			ast,
+			{
+				CallExpression: this.visitCallExpression,
+				Identifier: this.visitIdentifier,
+			},
+			undefined,
+			accessedBuiltIns,
+		);
 
-				// $(): This is not valid, ignore
-				if (node.arguments.length === 0) {
-					return;
-				}
+		return accessedBuiltIns;
+	}
 
-				const firstArg = node.arguments[0];
-				if (firstArg.type === 'Literal') {
-					if (typeof firstArg.value === 'string') {
-						// $("nodeName"): Static value, mark 'nodeName' as needed
-						accessedBuiltIns.markNodeAsNeeded(firstArg.value);
-					} else {
-						// $(123): Static value, but not a string --> invalid code --> ignore
-					}
-				} else {
-					// $(variable): Can't determine statically, mark all nodes as needed
-					accessedBuiltIns.markNeedsAllNodes();
-				}
+	private visitCallExpression = (
+		node: CallExpression,
+		state: BuiltInsParserState,
+		ancestors: Node[],
+	) => {
+		// $(...)
+		const isDollar = node.callee.type === 'Identifier' && node.callee.name === '$';
+		if (!isDollar) return;
 
-				// TODO: We could determine if $('node') is followed by a function call (e.g.
-				// first()) or a property (e.g. isExecuted) and only get the one accessed
-			},
+		// $(): This is not valid, ignore
+		if (node.arguments.length === 0) {
+			return;
+		}
 
-			Identifier(node) {
-				if (node.name === '$env') {
-					accessedBuiltIns.markEnvAsNeeded();
-				} else if (node.name === '$input' || node.name === '$json') {
-					accessedBuiltIns.markInputAsNeeded();
-				} else if (node.name === '$execution') {
-					accessedBuiltIns.markExecutionAsNeeded();
-				} else if (node.name === '$prevNode') {
-					accessedBuiltIns.markPrevNodeAsNeeded();
-				}
-			},
-		});
+		const firstArg = node.arguments[0];
+		if (!isLiteral(firstArg)) {
+			// $(variable): Can't easily determine statically, mark all nodes as needed
+			state.markNeedsAllNodes();
+			return;
+		}
 
-		return accessedBuiltIns;
+		if (typeof firstArg.value !== 'string') {
+			// $(123): Static value, but not a string --> invalid code --> ignore
+			return;
+		}
+
+		// $("node"): Static value, mark 'nodeName' as needed
+		state.markNodeAsNeeded(firstArg.value);
+
+		// Determine how $("node") is used
+		this.handlePrevNodeCall(node, state, ancestors);
+	};
+
+	private handlePrevNodeCall(_node: CallExpression, state: BuiltInsParserState, ancestors: Node[]) {
+		// $("node").item: In a case like this, the execution
+		// engine will traverse back from current node (i.e. the Code Node) to
+		// the "node" node and use `pairedItem`s to find which item is linked
+		// to the current item. So, we need to mark all nodes as needed.
+		// TODO: We could also mark all the nodes between the current node and
+		// the "node" node as needed, but that would require more complex logic.
+		const directParent = ancestors[ancestors.length - 2];
+		if (isMemberExpression(directParent)) {
+			const accessedProperty = directParent.property;
+
+			if (directParent.computed) {
+				// $("node")["item"]
+				if (isLiteral(accessedProperty)) {
+					if (accessedProperty.value === 'item') {
+						state.markNeedsAllNodes();
+					}
+					// Else: $("node")[123]: Static value, but not 'item' --> ignore
+				}
+				// $("node")[variable]
+				else if (isIdentifier(accessedProperty)) {
+					state.markNeedsAllNodes();
+				}
+			}
+			// $("node").item
+			else if (isIdentifier(accessedProperty) && accessedProperty.name === 'item') {
+				state.markNeedsAllNodes();
+			}
+		} else if (isVariableDeclarator(directParent) || isAssignmentExpression(directParent)) {
+			// const variable = $("node") or variable = $("node"):
+			// In this case we would need to track down all the possible use sites
+			// of 'variable' and determine if `.item` is accessed on it. This is
+			// more complex and skipped for now.
+			// TODO: Optimize for this case
+			state.markNeedsAllNodes();
+		} else {
+			// Something else than the cases above. Mark all nodes as needed as it
+			// could be a dynamic access.
+			state.markNeedsAllNodes();
+		}
 	}
+
+	private visitIdentifier = (node: Identifier, state: BuiltInsParserState) => {
+		if (node.name === '$env') {
+			state.markEnvAsNeeded();
+		} else if (node.name === '$input' || node.name === '$json') {
+			state.markInputAsNeeded();
+		} else if (node.name === '$execution') {
+			state.markExecutionAsNeeded();
+		} else if (node.name === '$prevNode') {
+			state.markPrevNodeAsNeeded();
+		}
+	};
 }