From 76a62bd0d3f544fc3e0dc514bb2fa763dda5d079 Mon Sep 17 00:00:00 2001 From: Craigory Coppola Date: Thu, 25 Apr 2024 16:45:26 -0400 Subject: [PATCH] fix(core): ensure create nodes functions are properly parallelized (#23005) (cherry picked from commit 0fd6d23e3f341002c4514110bc997817101940c6) --- docs/generated/devkit/CreateNodesContext.md | 4 +- .../src/project-graph/plugins/public-api.ts | 2 +- .../src/project-graph/plugins/utils.spec.ts | 123 ++++++++++++++++++ .../nx/src/project-graph/plugins/utils.ts | 67 +++++----- 4 files changed, 159 insertions(+), 37 deletions(-) create mode 100644 packages/nx/src/project-graph/plugins/utils.spec.ts diff --git a/docs/generated/devkit/CreateNodesContext.md b/docs/generated/devkit/CreateNodesContext.md index 25c357b8103e0c..861b8d7907eef0 100644 --- a/docs/generated/devkit/CreateNodesContext.md +++ b/docs/generated/devkit/CreateNodesContext.md @@ -6,7 +6,7 @@ Context for [CreateNodesFunction](../../devkit/documents/CreateNodesFunction) ### Properties -- [configFiles](../../devkit/documents/CreateNodesContext#configfiles): string[] +- [configFiles](../../devkit/documents/CreateNodesContext#configfiles): readonly string[] - [nxJsonConfiguration](../../devkit/documents/CreateNodesContext#nxjsonconfiguration): NxJsonConfiguration - [workspaceRoot](../../devkit/documents/CreateNodesContext#workspaceroot): string @@ -14,7 +14,7 @@ Context for [CreateNodesFunction](../../devkit/documents/CreateNodesFunction) ### configFiles -• `Readonly` **configFiles**: `string`[] +• `Readonly` **configFiles**: readonly `string`[] The subset of configuration files which match the createNodes pattern diff --git a/packages/nx/src/project-graph/plugins/public-api.ts b/packages/nx/src/project-graph/plugins/public-api.ts index 835ecf77c081d3..4810f7967132ee 100644 --- a/packages/nx/src/project-graph/plugins/public-api.ts +++ b/packages/nx/src/project-graph/plugins/public-api.ts @@ -22,7 +22,7 @@ export interface CreateNodesContext { /** * The subset of configuration files which match the createNodes pattern */ - readonly configFiles: string[]; + readonly configFiles: readonly string[]; } /** diff --git a/packages/nx/src/project-graph/plugins/utils.spec.ts b/packages/nx/src/project-graph/plugins/utils.spec.ts new file mode 100644 index 00000000000000..0e3916f9d92202 --- /dev/null +++ b/packages/nx/src/project-graph/plugins/utils.spec.ts @@ -0,0 +1,123 @@ +import { runCreateNodesInParallel } from './utils'; + +const configFiles = ['file1', 'file2'] as const; + +const context = { + file: 'file1', + nxJsonConfiguration: {}, + workspaceRoot: '', + configFiles, +} as const; + +describe('createNodesInParallel', () => { + it('should return results with context', async () => { + const plugin = { + name: 'test', + createNodes: [ + '*/**/*', + async (file: string) => { + return { + projects: { + [file]: { + root: file, + }, + }, + }; + }, + ], + } as const; + const options = {}; + + const results = await runCreateNodesInParallel( + configFiles, + plugin, + options, + context + ); + + expect(results).toMatchInlineSnapshot(` + [ + { + "file": "file1", + "pluginName": "test", + "projects": { + "file1": { + "root": "file1", + }, + }, + }, + { + "file": "file2", + "pluginName": "test", + "projects": { + "file2": { + "root": "file2", + }, + }, + }, + ] + `); + }); + + it('should handle async errors', async () => { + const plugin = { + name: 'test', + createNodes: [ + '*/**/*', + async () => { + throw new Error('Async Error'); + }, + ], + } as const; + const options = {}; + + const error = await runCreateNodesInParallel( + configFiles, + plugin, + options, + context + ).catch((e) => e); + + expect(error).toMatchInlineSnapshot( + `[AggregateCreateNodesError: Failed to create nodes]` + ); + + expect(error.errors).toMatchInlineSnapshot(` + [ + [CreateNodesError: The "test" plugin threw an error while creating nodes from file1:], + [CreateNodesError: The "test" plugin threw an error while creating nodes from file2:], + ] + `); + }); + + it('should handle sync errors', async () => { + const plugin = { + name: 'test', + createNodes: [ + '*/**/*', + () => { + throw new Error('Sync Error'); + }, + ], + } as const; + const options = {}; + + const error = await runCreateNodesInParallel( + configFiles, + plugin, + options, + context + ).catch((e) => e); + + expect(error).toMatchInlineSnapshot( + `[AggregateCreateNodesError: Failed to create nodes]` + ); + + expect(error.errors).toMatchInlineSnapshot(` + [ + [CreateNodesError: The "test" plugin threw an error while creating nodes from file1:], + [CreateNodesError: The "test" plugin threw an error while creating nodes from file2:], + ] + `); + }); +}); diff --git a/packages/nx/src/project-graph/plugins/utils.ts b/packages/nx/src/project-graph/plugins/utils.ts index c9196682793ba9..17ac3c0af49912 100644 --- a/packages/nx/src/project-graph/plugins/utils.ts +++ b/packages/nx/src/project-graph/plugins/utils.ts @@ -9,7 +9,12 @@ import type { LoadedNxPlugin, NormalizedPlugin, } from './internal-api'; -import type { CreateNodesContext, NxPlugin, NxPluginV2 } from './public-api'; +import { + CreateNodesResult, + type CreateNodesContext, + type NxPlugin, + type NxPluginV2, +} from './public-api'; import { AggregateCreateNodesError, CreateNodesError } from '../error-types'; export function isNxPluginV2(plugin: NxPlugin): plugin is NxPluginV2 { @@ -49,7 +54,7 @@ export function normalizeNxPlugin(plugin: NxPlugin): NormalizedPlugin { } export async function runCreateNodesInParallel( - configFiles: string[], + configFiles: readonly string[], plugin: NormalizedPlugin, options: unknown, context: CreateNodesContext @@ -59,39 +64,33 @@ export async function runCreateNodesInParallel( const errors: CreateNodesError[] = []; const results: CreateNodesResultWithContext[] = []; - const promises: Array> = configFiles.map((file) => { + const promises: Array> = configFiles.map(async (file) => { performance.mark(`${plugin.name}:createNodes:${file} - start`); - // Result is either static or a promise, using Promise.resolve lets us - // handle both cases with same logic - const value = Promise.resolve( - plugin.createNodes[1](file, options, context) - ); - return value - .catch((e) => { - performance.mark(`${plugin.name}:createNodes:${file} - end`); - errors.push( - new CreateNodesError({ - error: e, - pluginName: plugin.name, - file, - }) - ); - return null; - }) - .then((r) => { - performance.mark(`${plugin.name}:createNodes:${file} - end`); - performance.measure( - `${plugin.name}:createNodes:${file}`, - `${plugin.name}:createNodes:${file} - start`, - `${plugin.name}:createNodes:${file} - end` - ); - - // Existing behavior is to ignore null results of - // createNodes function. - if (r) { - results.push({ ...r, file, pluginName: plugin.name }); - } - }); + try { + const value = await plugin.createNodes[1](file, options, context); + if (value) { + results.push({ + ...value, + file, + pluginName: plugin.name, + }); + } + } catch (e) { + errors.push( + new CreateNodesError({ + error: e, + pluginName: plugin.name, + file, + }) + ); + } finally { + performance.mark(`${plugin.name}:createNodes:${file} - end`); + performance.measure( + `${plugin.name}:createNodes:${file}`, + `${plugin.name}:createNodes:${file} - start`, + `${plugin.name}:createNodes:${file} - end` + ); + } }); await Promise.all(promises).then(() => {