refactor(core): Move WorkflowHooks to core, improve type-safety, and add tests (no-changelog) #12364

netroy · 2024-12-24T19:51:08Z

Summary

This PR

Renames WorkflowHooks to ExecutionHooks
moves it from n8n-workflow to n8n-core
Creates a new ExecutionHooksFactory class, and moves all execution hooks code out of workflow-execute-additional-data
makes all of these hooks type-safe
adds tests for the vast majority of this code.

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
Tests included.
PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

…add tests

codecov · 2024-12-25T09:45:59Z

Codecov Report

Attention: Patch coverage is 75.98684% with 73 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/cli/src/workflow-runner.ts	57.57%	25 Missing and 3 partials ⚠️
...ecution-lifecycle-hooks/execution-hooks-factory.ts	87.73%	11 Missing and 9 partials ⚠️
packages/cli/src/utils/object-to-error.ts	55.55%	6 Missing and 2 partials ⚠️
packages/core/src/WorkflowExecute.ts	27.27%	8 Missing ⚠️
packages/cli/src/scaling/job-processor.ts	33.33%	4 Missing ⚠️
packages/core/src/TriggersAndPollers.ts	71.42%	2 Missing ⚠️
packages/core/src/index.ts	0.00%	1 Missing ⚠️
...core/src/node-execution-context/execute-context.ts	0.00%	1 Missing ⚠️
.../src/node-execution-context/supply-data-context.ts	50.00%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

…hooks

ivov

I'll come back later to finish reviewing and do some testing.

ivov · 2025-01-03T14:58:08Z

packages/cli/src/workflow-runner.ts

@@ -136,14 +142,12 @@ export class WorkflowRunner {
 		try {
 			await this.permissionChecker.check(workflowId, nodes);
 		} catch (error) {
+			const executionHooksFactory = Container.get(ExecutionHooksFactory);


Why are we not injecting ExecutionHooksFactory in the constructor? Same below.

cyclic dependency. we needs a some serious rearchitecting/refactoring of this code to break that cycles.

ivov · 2025-01-03T15:00:32Z

packages/cli/src/workflow-runner.ts

-					this.activeExecutions.resolveResponsePromise(executionId, response);
-				},
-			];
+			additionalData.hooks.addHook('sendResponse', async (response) => {


Can we make addHook accept sync and async functions? For less noise. Applies in a lot of other places as well.

Suggested change

additionalData.hooks.addHook('sendResponse', async (response) => {

additionalData.hooks.addHook('sendResponse', (response) => {

ivov · 2025-01-03T15:02:03Z

packages/cli/src/workflow-runner.ts

 		// TODO: For realtime jobs should probably also not do retry or not retry if they are older than x seconds.
 		//       Check if they get retried by default and how often.
 		let job: Job;
-		let hooks: WorkflowHooks;
+		let lifecycleHooks: ExecutionHooks;


See note on naming at execution-hooks.ts

ivov · 2025-01-03T15:07:17Z

packages/core/src/Interfaces.ts

+declare module 'n8n-workflow' {
+	interface IWorkflowExecuteAdditionalData {
+		hooks?: ExecutionHooks;
+	}
+}
+


Come back later to check if this is needed.

ivov · 2025-01-03T15:07:43Z

packages/core/src/TriggersAndPollers.ts

@@ -46,26 +46,20 @@ export class TriggersAndPollers {

 			// Add the manual trigger response which resolves when the first time data got emitted
 			triggerResponse!.manualTriggerResponse = new Promise((resolve, reject) => {
+				const hooks = additionalData.hooks!;


Can we assert() instead of !?

we can. but I actually prefer to fix the typing, since these should always be available. The issue is that we've always hacked execution engine to do credential testing, and to keep the that legacy credential testing mechanism, we need to keep this optional.
Maybe we should redo credential testing first? 🤔

ivov · 2025-01-03T15:47:50Z

packages/cli/src/execution-lifecycle-hooks/execution-hooks-factory.ts

+		return hooks;
+	}
+
+	private addPreExecuteHooks(hooks: ExecutionHooks) {


It's called PreExecute but adds a nodeExecuteAfter callback? Can we find a better name for this?

I tried, but the only solution I came up was to break this up method into separate methods, and name those appropriately. I'd prefer to do that in another PR.

ivov · 2025-01-03T15:49:07Z

packages/cli/src/execution-lifecycle-hooks/execution-hooks-factory.ts

+		});
+	}
+
+	private addEventHooks(hooks: ExecutionHooks) {


Named like this I'd expect to see all events (i.e. points in execution lifecycle) but the important one workflowExecuteAfter is missing here.

ivov · 2025-01-03T15:52:36Z

packages/cli/src/execution-lifecycle-hooks/execution-hooks-factory.ts

+		});
+	}
+
+	/** Returns hook functions to push data to Editor-UI */


Suggested change

/** Returns hook functions to push data to Editor-UI */

/** Add hook callbacks to push live updates to the frontend. */

ivov · 2025-01-03T15:55:14Z

packages/core/src/execution-hooks.ts

+		}
+	}
+
+	addHook<Hook extends keyof RegisteredHooks>(


Suggestion - In my mind, the hook is the spot in the lifecycle where we run one or more registered callbacks.

Suggested change

addHook<Hook extends keyof RegisteredHooks>(

addCallback<Hook extends keyof RegisteredHooks>(

ivov · 2025-01-03T16:02:16Z

packages/cli/src/execution-lifecycle-hooks/execution-hooks-factory.ts

+	/** Returns hook functions to save workflow execution and call error workflow */
+	private addSavingHooks(hooks: ExecutionHooks, isWorker: boolean) {


We'll need much more work until hooks are organized in a way that makes sense, but until we get there maybe we can use more accurate names, even rather long.

Suggested change

/** Returns hook functions to save workflow execution and call error workflow */

private addSavingHooks(hooks: ExecutionHooks, isWorker: boolean) {

private addWorkflowExecuteAfterCallback(hooks: ExecutionHooks, isWorker: boolean) {

n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team node/improvement New feature or request labels Dec 24, 2024

refactor(core): Move WorkflowHooks to core, improve type-safety, and …

8018023

…add tests

netroy force-pushed the refactor-execution-hooks branch from da7898e to 8018023 Compare December 25, 2024 09:21

netroy marked this pull request as ready for review December 30, 2024 16:33

netroy changed the title ~~refactor(core): Move WorkflowHooks to core, improve type-safety, and add tests~~ refactor(core): Move WorkflowHooks to core, improve type-safety, and add tests (no-changelog) Dec 30, 2024

Merge remote-tracking branch 'origin/master' into refactor-execution-…

f3cbdaf

…hooks

netroy force-pushed the refactor-execution-hooks branch from 0ac5410 to f3cbdaf Compare December 30, 2024 16:41

Merge remote-tracking branch 'origin/master' into refactor-execution-…

d9a8229

…hooks

netroy requested a review from ivov January 2, 2025 11:57

ivov reviewed Jan 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): Move WorkflowHooks to core, improve type-safety, and add tests (no-changelog) #12364

refactor(core): Move WorkflowHooks to core, improve type-safety, and add tests (no-changelog) #12364

netroy commented Dec 24, 2024 •

edited

Loading

codecov bot commented Dec 25, 2024

ivov left a comment

ivov Jan 3, 2025

netroy Jan 3, 2025

ivov Jan 3, 2025

ivov Jan 3, 2025

ivov Jan 3, 2025

ivov Jan 3, 2025

netroy Jan 3, 2025

ivov Jan 3, 2025 •

edited

Loading

netroy Jan 3, 2025

ivov Jan 3, 2025

ivov Jan 3, 2025

ivov Jan 3, 2025 •

edited

Loading

ivov Jan 3, 2025

	additionalData.hooks.addHook('sendResponse', async (response) => {
	additionalData.hooks.addHook('sendResponse', (response) => {

	/** Returns hook functions to push data to Editor-UI */
	/** Add hook callbacks to push live updates to the frontend. */

	addHook<Hook extends keyof RegisteredHooks>(
	addCallback<Hook extends keyof RegisteredHooks>(

		/** Returns hook functions to save workflow execution and call error workflow */
		private addSavingHooks(hooks: ExecutionHooks, isWorker: boolean) {

	/** Returns hook functions to save workflow execution and call error workflow */
	private addSavingHooks(hooks: ExecutionHooks, isWorker: boolean) {
	private addWorkflowExecuteAfterCallback(hooks: ExecutionHooks, isWorker: boolean) {

refactor(core): Move WorkflowHooks to core, improve type-safety, and add tests (no-changelog) #12364

Are you sure you want to change the base?

refactor(core): Move WorkflowHooks to core, improve type-safety, and add tests (no-changelog) #12364

Conversation

netroy commented Dec 24, 2024 • edited Loading

Summary

Related Linear tickets, Github issues, and Community forum posts

Review / Merge checklist

codecov bot commented Dec 25, 2024

Codecov Report

ivov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivov Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

netroy commented Dec 24, 2024 •

edited

Loading

ivov Jan 3, 2025 •

edited

Loading

ivov Jan 3, 2025 •

edited

Loading