fix: Skip dependency files and continue after custom node load (#41117)

Co-authored-by: n8n-cat-bot[bot] <n8n-cat-bot[bot]@users.noreply.github.com>

packages/core/src/nodes-loader/__tests__/custom-directory-loader.dependencies.test.ts

@@ -0,0 +1,368 @@
+import { Logger } from '@n8n/backend-common';
+import {
+	mkdirSync,
+	mkdtempSync,
+	realpathSync,
+	rmSync,
+	symlinkSync,
+	unlinkSync,
+	writeFileSync,
+} from 'node:fs';
+import { tmpdir } from 'node:os';
+import path from 'node:path';
+
+import { mockInstance } from '@test/utils';
+
+import { CustomDirectoryLoader } from '../custom-directory-loader';
+
+// NODE-6005: Dependency build files can use n8n node and credential file suffixes.
+describe('CustomDirectoryLoader dependencies', () => {
+	const logger = mockInstance(Logger);
+
+	let tmpRoot: string;
+	let customDir: string;
+	let loader: CustomDirectoryLoader;
+
+	const writeFile = (root: string, relativePath: string, source: string) => {
+		const filePath = path.join(root, relativePath);
+		mkdirSync(path.dirname(filePath), { recursive: true });
+		writeFileSync(filePath, source);
+		return filePath;
+	};
+
+	const writeCustomFiles = (root: string) => {
+		const nodePath = writeFile(
+			root,
+			'nodes/Foo.node.js',
+			`module.exports.Foo = class Foo {
+				constructor() {
+					this.description = {
+						displayName: 'Foo',
+						name: 'foo',
+						group: ['transform'],
+						version: 1,
+						description: 'Test node',
+						defaults: { name: 'Foo' },
+						inputs: ['main'],
+						outputs: ['main'],
+						credentials: [{ name: 'fooApi', required: true }],
+						properties: [],
+					};
+				}
+			};`,
+		);
+		const credentialPath = writeFile(
+			root,
+			'credentials/FooApi.credentials.js',
+			`module.exports.FooApi = class FooApi {
+				constructor() {
+					this.name = 'fooApi';
+					this.displayName = 'Foo API';
+					this.properties = [];
+				}
+			};`,
+		);
+		return { nodePath, credentialPath };
+	};
+
+	const writeDependency = (root: string, suffix: string) => {
+		writeFile(root, 'node_modules/wmf/package.json', JSON.stringify({ name: 'wmf' }));
+		return writeFile(root, `node_modules/wmf/dist/wmf.${suffix}.js`, 'module.exports = {};');
+	};
+
+	const expectCustomFilesLoaded = (files: { nodePath: string; credentialPath: string }) => {
+		expect(loader.packageName).toBe('CUSTOM');
+		expect(Object.keys(loader.nodeTypes)).toEqual(['foo']);
+		expect(Object.keys(loader.credentialTypes)).toEqual(['fooApi']);
+		expect(loader.getNode('foo').sourcePath).toBe(files.nodePath);
+		expect(loader.getCredential('fooApi').sourcePath).toBe(files.credentialPath);
+	};
+
+	beforeEach(() => {
+		vi.clearAllMocks();
+		tmpRoot = realpathSync(mkdtempSync(path.join(tmpdir(), 'n8n-custom-dependencies-')));
+		customDir = path.join(tmpRoot, 'custom');
+		mkdirSync(customDir);
+		loader = new CustomDirectoryLoader(customDir);
+	});
+
+	afterEach(() => {
+		loader.reset();
+		vi.restoreAllMocks();
+		rmSync(tmpRoot, { recursive: true, force: true });
+	});
+
+	it('loads loose custom nodes and credentials without dependencies', async () => {
+		const files = writeCustomFiles(customDir);
+
+		await loader.loadAll();
+
+		expectCustomFilesLoaded(files);
+	});
+
+	describe.each(['loose', 'package'])('%s custom files', (layout) => {
+		it.each(['single-version', 'versioned'])(
+			'skips a %s node with an invalid description without registering it',
+			async (kind) => {
+				const root =
+					layout === 'loose' ? customDir : path.join(customDir, 'node_modules/n8n-nodes-foo');
+				const files = writeCustomFiles(root);
+				const brokenPath = writeFile(
+					root,
+					'AaaBroken.node.js',
+					`module.exports.AaaBroken = class AaaBroken {
+						constructor() {
+							this.description = {
+								displayName: 'Broken',
+								name: 'broken',
+								group: ['transform'],
+								version: 1,
+								description: 'Test node',
+								defaults: { name: 'Broken' },
+								inputs: ['main'],
+								outputs: ['main'],
+								credentials: [{ name: 'brokenApi', required: true }],
+								properties: [{
+									displayName: 'Operation',
+									name: 'operation',
+									type: 'options',
+									default: 'test',
+									options: [{ name: 'Test', value: 'test' }],
+									builderHint: { searchHint: 'Test hint' },
+								}],
+							};
+							if (${kind === 'versioned'}) {
+								this.currentVersion = 2;
+								this.nodeVersions = {
+									1: { description: this.description },
+									2: { description: { ...this.description, version: 2, properties: [] } },
+								};
+							}
+						}
+					};`,
+				);
+				if (layout === 'package') {
+					writeFile(
+						root,
+						'package.json',
+						JSON.stringify({
+							name: 'n8n-nodes-foo',
+							n8n: {
+								nodes: ['AaaBroken.node.js', 'nodes/Foo.node.js'],
+								credentials: ['credentials/FooApi.credentials.js'],
+							},
+						}),
+					);
+				}
+
+				await expect(loader.loadAll()).resolves.toBeUndefined();
+
+				expectCustomFilesLoaded(files);
+				expect(Object.keys(loader.known.nodes)).toEqual(['foo']);
+				expect(loader.loadedNodes).toEqual([{ name: 'foo', version: 1 }]);
+				expect(loader.types.nodes.map(({ name }) => name)).toEqual(['foo']);
+				expect(loader.nodesByCredential).toEqual({ fooApi: ['foo'] });
+				expect(() => loader.getNode('broken')).toThrow();
+				expect(logger.warn).toHaveBeenCalledTimes(1);
+				expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining(brokenPath), {
+					error: expect.objectContaining({
+						message: expect.stringContaining('has a builderHint on the "operation" discriminator'),
+					}),
+				});
+			},
+		);
+
+		describe.each(['node', 'credentials'])('*.%s.js', (suffix) => {
+			it.each([
+				{
+					failure: 'a missing dependency',
+					source: "require('n8n-test-missing-dependency');",
+					error: expect.objectContaining({ code: 'MODULE_NOT_FOUND' }),
+				},
+				{
+					failure: 'an incorrect class name',
+					source: 'module.exports = {};',
+					error: expect.objectContaining({
+						message: 'Class could not be found. Please check if the class is named correctly.',
+					}),
+				},
+			])('skips a file with $failure and loads healthy files', async ({ source, error }) => {
+				const root =
+					layout === 'loose' ? customDir : path.join(customDir, 'node_modules/n8n-nodes-foo');
+				const files = writeCustomFiles(root);
+				const brokenPath = writeFile(root, `AaaBroken.${suffix}.js`, source);
+				if (layout === 'package') {
+					writeFile(
+						root,
+						'package.json',
+						JSON.stringify({
+							name: 'n8n-nodes-foo',
+							n8n: {
+								nodes: [
+									...(suffix === 'node' ? [path.basename(brokenPath)] : []),
+									'nodes/Foo.node.js',
+								],
+								credentials: [
+									...(suffix === 'credentials' ? [path.basename(brokenPath)] : []),
+									'credentials/FooApi.credentials.js',
+								],
+							},
+						}),
+					);
+				}
+
+				await expect(loader.loadAll()).resolves.toBeUndefined();
+
+				expectCustomFilesLoaded(files);
+				expect(logger.warn).toHaveBeenCalledTimes(1);
+				expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining(brokenPath), { error });
+			});
+		});
+	});
+
+	it.each(['node', 'credentials'])(
+		'ignores dependency *.%s.js files beside loose custom files',
+		async (suffix) => {
+			const files = writeCustomFiles(customDir);
+			const dependencyPath = writeDependency(customDir, suffix);
+			const loadNode = vi.spyOn(loader, 'loadNodeFromFile');
+			const loadCredential = vi.spyOn(loader, 'loadCredentialFromFile');
+
+			await expect(loader.loadAll()).resolves.toBeUndefined();
+
+			expectCustomFilesLoaded(files);
+			expect(loadNode).not.toHaveBeenCalledWith(dependencyPath);
+			expect(loadCredential).not.toHaveBeenCalledWith(dependencyPath);
+		},
+	);
+
+	it('skips ordinary dependencies without a warning', async () => {
+		const files = writeCustomFiles(customDir);
+		writeDependency(customDir, 'node');
+
+		await loader.loadAll();
+
+		expectCustomFilesLoaded(files);
+		expect(logger.warn).not.toHaveBeenCalled();
+		expect(logger.debug).toHaveBeenCalledWith(
+			`Skipping custom node package "${path.join(customDir, 'node_modules/wmf/package.json')}": no n8n field found.`,
+		);
+	});
+
+	it('skips a broken manifest and loads another custom package', async () => {
+		const packageFile = writeFile(customDir, 'node_modules/a-broken/package.json', '{ "n8n":');
+		const packageDir = path.join(customDir, 'node_modules/z-working');
+		const files = writeCustomFiles(packageDir);
+		writeFile(
+			packageDir,
+			'package.json',
+			JSON.stringify({
+				name: 'z-working',
+				n8n: {
+					nodes: ['nodes/Foo.node.js'],
+					credentials: ['credentials/FooApi.credentials.js'],
+				},
+			}),
+		);
+
+		await expect(loader.loadAll()).resolves.toBeUndefined();
+
+		expectCustomFilesLoaded(files);
+		expect(logger.warn).toHaveBeenCalledTimes(1);
+		expect(logger.warn).toHaveBeenCalledWith(expect.stringContaining(packageFile), {
+			error: expect.any(Error),
+		});
+	});
+
+	it.each(['nodePath', 'credentialPath'] as const)(
+		'skips a linked package with a missing %s until it is built',
+		async (missingFile) => {
+			const looseFiles = writeCustomFiles(customDir);
+			const projectDir = path.join(tmpRoot, 'project');
+			const projectFiles = writeCustomFiles(path.join(projectDir, 'dist'));
+			unlinkSync(projectFiles[missingFile]);
+			writeFile(
+				projectDir,
+				'package.json',
+				JSON.stringify({
+					name: 'n8n-nodes-foo',
+					n8n: {
+						nodes: ['dist/nodes/Foo.node.js'],
+						credentials: ['dist/credentials/FooApi.credentials.js'],
+					},
+				}),
+			);
+			const packageLink = path.join(customDir, 'node_modules/n8n-nodes-foo');
+			mkdirSync(path.dirname(packageLink), { recursive: true });
+			symlinkSync(projectDir, packageLink, 'dir');
+			const linkedFiles = {
+				nodePath: path.join(packageLink, 'dist/nodes/Foo.node.js'),
+				credentialPath: path.join(packageLink, 'dist/credentials/FooApi.credentials.js'),
+			};
+			const loadNode = vi.spyOn(loader, 'loadNodeFromFile');
+			const loadCredential = vi.spyOn(loader, 'loadCredentialFromFile');
+
+			await expect(loader.loadAll()).resolves.toBeUndefined();
+
+			expectCustomFilesLoaded(looseFiles);
+			expect(loadNode).not.toHaveBeenCalledWith(linkedFiles.nodePath);
+			expect(loadCredential).not.toHaveBeenCalledWith(linkedFiles.credentialPath);
+			expect(logger.warn).toHaveBeenCalledTimes(1);
+			expect(logger.warn).toHaveBeenCalledWith(
+				expect.stringContaining(path.join(packageLink, 'package.json')),
+				{ error: expect.objectContaining({ code: 'ENOENT' }) },
+			);
+
+			writeCustomFiles(path.join(projectDir, 'dist'));
+			loader.reset();
+			vi.clearAllMocks();
+			await loader.loadAll();
+
+			expectCustomFilesLoaded(linkedFiles);
+			expect(logger.warn).not.toHaveBeenCalled();
+		},
+	);
+
+	it.each(['n8n-nodes-foo', '@scope/n8n-nodes-foo'])(
+		'loads the symlinked dev package %s without loading its dependencies',
+		async (packageName) => {
+			const projectDir = path.join(tmpRoot, 'project');
+			writeCustomFiles(path.join(projectDir, 'dist'));
+			writeFile(
+				projectDir,
+				'package.json',
+				JSON.stringify({
+					name: packageName,
+					version: '1.0.0',
+					n8n: {
+						nodes: ['dist/nodes/Foo.node.js'],
+						credentials: ['dist/credentials/FooApi.credentials.js'],
+					},
+				}),
+			);
+			writeDependency(projectDir, 'node');
+			writeDependency(projectDir, 'credentials');
+			writeFile(projectDir, 'dist/nodes/Unlisted.node.js', 'module.exports = {};');
+			writeFile(projectDir, 'dist/credentials/Unlisted.credentials.js', 'module.exports = {};');
+			const packageLink = path.join(customDir, 'node_modules', packageName);
+			mkdirSync(path.dirname(packageLink), { recursive: true });
+			symlinkSync(projectDir, packageLink, 'dir');
+			const loadNode = vi.spyOn(loader, 'loadNodeFromFile');
+			const loadCredential = vi.spyOn(loader, 'loadCredentialFromFile');
+
+			await expect(loader.loadAll()).resolves.toBeUndefined();
+
+			expect(loader.packageName).toBe('CUSTOM');
+			expect(Object.keys(loader.nodeTypes)).toEqual(['foo']);
+			expect(Object.keys(loader.credentialTypes)).toEqual(['fooApi']);
+			expect(loader.getNode('foo').type.description.name).toBe('foo');
+			expect(loader.getCredential('fooApi').type.name).toBe('fooApi');
+			expect(loadNode).not.toHaveBeenCalledWith(
+				path.join(packageLink, 'node_modules/wmf/dist/wmf.node.js'),
+			);
+			expect(loadCredential).not.toHaveBeenCalledWith(
+				path.join(packageLink, 'node_modules/wmf/dist/wmf.credentials.js'),
+			);
+		},
+	);
+});

packages/core/src/nodes-loader/__tests__/custom-directory-loader.hot-reload.test.ts

@@ -41,6 +41,13 @@ describe('CustomDirectoryLoader hot reload (NODE-5225)', () => {
 		return file;
 	};
 
+	const writePackage = (dir: string, name: string) => {
+		fs.writeFileSync(
+			path.join(dir, 'package.json'),
+			JSON.stringify({ name, n8n: { nodes: ['dist/nodes/Reloadable.node.js'] } }),
+		);
+	};
+
 	// The node description type is a union; the fixture is a non-versioned node.
 	const loadedVersion = (loader: CustomDirectoryLoader) =>
 		(loader.getNode('reloadable').type.description as INodeTypeDescription).version;
@@ -59,6 +66,7 @@ describe('CustomDirectoryLoader hot reload (NODE-5225)', () => {
 		// The developer's node project, compiled to `dist`.
 		const projectDir = path.join(tmpRoot, 'my-node-project');
 		const nodeFile = writeNode(path.join(projectDir, 'dist', 'nodes'), 1);
+		writePackage(projectDir, 'n8n-nodes-reloadable');
 
 		// The project is symlinked into node_modules, exactly how `n8n-node dev`
 		// installs the node for live preview.
@@ -86,6 +94,7 @@ describe('CustomDirectoryLoader hot reload (NODE-5225)', () => {
 		// one level deeper, under `node_modules/@scope/<pkg>`.
 		const projectDir = path.join(tmpRoot, 'my-scoped-node-project');
 		const nodeFile = writeNode(path.join(projectDir, 'dist', 'nodes'), 1);
+		writePackage(projectDir, '@scope/n8n-nodes-reloadable');
 
 		const scopeDir = path.join(customDir, 'node_modules', '@scope');
 		fs.mkdirSync(scopeDir, { recursive: true });

packages/core/src/nodes-loader/__tests__/directory-loader.test.ts

@@ -18,7 +18,8 @@ const mockFs = mock(fs);
 const mockFsPromises = mock(fsPromises);
 
 vi.mock('fast-glob', () => ({
-	default: async (pattern: string) => {
+	default: async (pattern: string | string[]) => {
+		if (Array.isArray(pattern)) return [];
 		return pattern.endsWith('.node.js')
 			? ['dist/Node1/Node1.node.js', 'dist/Node2/Node2.node.js']
 			: ['dist/Credential1.js'];

packages/core/src/nodes-loader/custom-directory-loader.ts

@@ -1,7 +1,11 @@
 import glob from 'fast-glob';
+import { jsonParse } from 'n8n-workflow';
+import { access, readFile } from 'node:fs/promises';
+import path from 'node:path';
 
 import { CUSTOM_NODES_PACKAGE_NAME } from './constants';
 import { DirectoryLoader } from './directory-loader';
+import type { n8n } from './types';
 
 /**
  * Loader for source files of nodes and credentials located in a custom dir,
@@ -21,19 +25,66 @@ export class CustomDirectoryLoader extends DirectoryLoader {
 		const nodes = await glob('**/*.node.js', {
 			cwd: this.directory,
 			absolute: true,
+			ignore: ['**/node_modules/**'],
 		});
 
-		for (const nodePath of nodes) {
-			this.loadNodeFromFile(nodePath);
-		}
-
 		const credentials = await glob('**/*.credentials.js', {
 			cwd: this.directory,
 			absolute: true,
+			ignore: ['**/node_modules/**'],
 		});
 
+		// Dev packages are linked here. Load their manifest entries, not their dependencies.
+		const packageFiles = await glob(
+			['node_modules/*/package.json', 'node_modules/@*/*/package.json'],
+			{ cwd: this.directory, absolute: true },
+		);
+		for (const packageFile of packageFiles) {
+			try {
+				const { n8n } = jsonParse<n8n.PackageJson>(await readFile(packageFile, 'utf8'));
+				if (!n8n) {
+					this.logger.debug(`Skipping custom node package "${packageFile}": no n8n field found.`);
+					continue;
+				}
+
+				const packageDirectory = path.dirname(packageFile);
+				const packageNodes = Array.isArray(n8n.nodes)
+					? n8n.nodes.map((nodePath) => path.resolve(packageDirectory, nodePath))
+					: [];
+				const packageCredentials = Array.isArray(n8n.credentials)
+					? n8n.credentials.map((credentialPath) => path.resolve(packageDirectory, credentialPath))
+					: [];
+
+				// Skip the whole package if a build has not produced all its manifest entries.
+				await Promise.all(
+					[...packageNodes, ...packageCredentials].map(async (filePath) => await access(filePath)),
+				);
+				nodes.push(...packageNodes);
+				credentials.push(...packageCredentials);
+			} catch (error) {
+				this.logger.warn(
+					`Skipping custom node package "${packageFile}": failed to read its manifest or find its files.`,
+					{ error },
+				);
+			}
+		}
+
+		for (const nodePath of nodes) {
+			try {
+				this.loadNodeFromFile(nodePath);
+			} catch (error) {
+				this.logger.warn(`Skipping custom node "${nodePath}": failed to load.`, { error });
+			}
+		}
+
 		for (const credentialPath of credentials) {
-			this.loadCredentialFromFile(credentialPath);
+			try {
+				this.loadCredentialFromFile(credentialPath);
+			} catch (error) {
+				this.logger.warn(`Skipping custom credential "${credentialPath}": failed to load.`, {
+					error,
+				});
+			}
 		}
 	}
 }

packages/core/src/nodes-loader/directory-loader.ts

@@ -211,6 +211,11 @@ export abstract class DirectoryLoader implements NodeLoader {
 				: tempNode.description.version;
 		}
 
+		const nodeDescriptions = this.getVersionedNodeTypeAll(tempNode).map(
+			({ description }) => description,
+		);
+		nodeDescriptions.forEach(validateNodeDescription);
+
 		this.known.nodes[nodeType] = {
 			className: tempNode.constructor.name,
 			sourcePath: filePath,
@@ -226,10 +231,7 @@ export abstract class DirectoryLoader implements NodeLoader {
 			version: nodeVersion,
 		});
 
-		this.getVersionedNodeTypeAll(tempNode).forEach(({ description }) => {
-			validateNodeDescription(description);
-			this.types.nodes.push(description);
-		});
+		this.types.nodes.push(...nodeDescriptions);
 
 		for (const credential of this.getCredentialsForNode(tempNode)) {
 			if (!this.nodesByCredential[credential.name]) {