Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(core): Decouple event bus from internal hooks (no-changelog) #9724

Merged
merged 29 commits into from
Jun 20, 2024
Merged

refactor(core): Decouple event bus from internal hooks (no-changelog) #9724

merged 29 commits into from
Jun 20, 2024

Conversation

ivov
Copy link
Contributor

@ivov ivov commented Jun 13, 2024 •
edited
Loading

To decouple MessageEventBus from InternalHooks, set up EventRelay to emit events from various services and AuditEventRelay to relay those events to MessageEventBus. In future, decouple telemetry and tear down InternalHooks.

  • Calls to InternalHooks and EventRelay will coexist until we decouple telemetry from InternalHooks.
  • Internal hooks that existed only for the event bus have been deleted, as they are now covered by events: onNodeBeforeExecute, onNodePostExecute, onWorkflowBeforeExecute, onUserLoginSuccess, onUserLoginFailed
  • Internal hooks that were unused have been deleted: onWorkflowCrashed, onUserReinvite
  • Internal hooks that existed for both telemetry and event bus remain only with telemetry.

Follow-up to: #9697

@n8n-assistant n8n-assistant bot added core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team labels Jun 13, 2024
@ivov ivov marked this pull request as ready for review June 14, 2024 12:52
@ivov ivov mentioned this pull request Jun 17, 2024
Merged
tomi
tomi previously approved these changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@tomi tomi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing the feedback, looking good 👏 Left one more question about the error type we are throwing.

packages/cli/src/errors/redactable.error.ts Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

✅ No visual regressions found.

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@ivov
Copy link
Contributor Author

ivov commented Jun 18, 2024

@tomi Thanks for the thorough review! Did a final comb-through and found a missing property, if you can please reapprove?

@tomi
Copy link
Contributor

tomi commented Jun 18, 2024

@ivov looks like there's a conflict that should be resolved first

tomi
tomi previously approved these changes Jun 18, 2024
View reviewed changes
Copy link
Contributor

@tomi tomi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@github-actions GitHub Actions
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

@ivov
Copy link
Contributor Author

ivov commented Jun 18, 2024 •
edited
Loading

e2e surfaced a dep cycle, will check soon

2024-06-18T14:04:31.946Z | error    | ReferenceError: Circular dependency: TargetWorkflowExecutionService. Index: 6
    at /Users/ivov/Development/n8n/node_modules/.pnpm/[email protected]_patch_hash=sk6omkefrosihg7lmqbzh7vfxe/node_modules/src/container-instance.class.ts:316:9
    at Array.map (<anonymous>)
    at ContainerInstance.initializeParams (/Users/ivov/Development/n8n/node_modules/.pnpm/[email protected]_patch_hash=sk6omkefrosihg7lmqbzh7vfxe/node_modules/src/container-instance.class.ts:314:23)
    at ContainerInstance.getServiceValue (/Users/ivov/Development/n8n/node_modules/.pnpm/[email protected]_patch_hash=sk6omkefrosihg7lmqbzh7vfxe/node_modules/src/container-instance.class.ts:277:27)
    at ContainerInstance.get (/Users/ivov/Development/n8n/node_modules/.pnpm/[email protected]_patch_hash=sk6omkefrosihg7lmqbzh7vfxe/node_modules/src/container-instance.class.ts:54:36)
    at /Users/ivov/Development/n8n/node_modules/.pnpm/[email protected]_patch_hash=sk6omkefrosihg7lmqbzh7vfxe/node_modules/src/container-instance.class.ts:338:8
    at Array.map (<anonymous>)
    at ContainerInstance.initializeParams (/Users/ivov/Development/n8n/node_modules/.pnpm/[email protected]_patch_hash=sk6omkefrosihg7lmqbzh7vfxe/node_modules/src/container-instance.class.ts:314:23)
    at ContainerInstance.getServiceValue (/Users/ivov/Development/n8n/node_modules/.pnpm/[email protected]_patch_hash=sk6omkefrosihg7lmqbzh7vfxe/node_modules/src/container-instance.class.ts:277:27)
    at ContainerInstance.get (/Users/ivov/Development/n8n/node_modules/.pnpm/[email protected]_patch_hash=sk6omkefrosihg7lmqbzh7vfxe/node_modules/src/container-instance.class.ts:54:36) "{ file: 'start.js', function: 'catch' }"

Edit: Discussion here

@ivov ivov requested a review from tomi June 20, 2024 10:20
@cypress Cypress
Copy link

cypress bot commented Jun 20, 2024

3 flaky tests on run #5603 ↗︎

0 395 0 0 Flakiness 3

Details:

🌳 🖥️ browsers:node18.12.0-chrome107 🤖 ivov 🗃️ e2e/*
Project: n8n Commit: 68d4dc3b0e
Status: Passed Duration: 04:42 💡
Started: Jun 20, 2024 10:26 AM Ended: Jun 20, 2024 10:31 AM
Flakiness  5-ndv.cy.ts • 2 flaky tests

View Output Video

Test Artifacts
NDV > should not retrieve remote options when required params throw errors Screenshots Video
NDV > Stop listening for trigger event from NDV Screenshots Video
Flakiness  10-undo-redo.cy.ts • 1 flaky test

View Output Video

Test Artifacts
Undo/Redo > should undo/redo adding connected nodes Test Replay Screenshots Video

Review all test suite changes for PR #9724 ↗︎

@github-actions GitHub Actions
Copy link
Contributor

✅ All Cypress E2E specs passed

@ivov ivov merged commit 199dff4 into master Jun 20, 2024
26 checks passed
@ivov ivov deleted the decouple-event-bus-from-internal-hooks branch June 20, 2024 10:32
adrian-martinez-onestic pushed a commit to onesdata/n8n-fork that referenced this pull request Jun 20, 2024
@ivov @adrian-martinez-onestic
refactor(core): Decouple event bus from internal hooks (no-changelog) (…
1b3cfc9 
…n8n-io#9724)
adrian-martinez-onestic pushed a commit to onesdata/n8n-fork that referenced this pull request Jun 20, 2024
@ivov @adrian-martinez-onestic
refactor(core): Decouple event bus from internal hooks (no-changelog) (…
714c7fd 
…n8n-io#9724)
@janober
Copy link
Member

janober commented Jun 20, 2024

Got released with [email protected]

1 similar comment
@janober
Copy link
Member

janober commented Jun 20, 2024

Got released with [email protected]

ivov added a commit that referenced this pull request Jun 24, 2024
@ivov
fix(core): Fix init for AuditEventRelay
11a1023 
Followup to #9724
@ivov ivov mentioned this pull request Jun 24, 2024
Merged
@ivov ivov mentioned this pull request Jul 17, 2024
Merged
@ivov ivov mentioned this pull request Aug 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomi tomi tomi approved these changes

Assignees
No one assigned
Labels
core Enhancement outside /nodes-base and /editor-ui n8n team Authored by the n8n team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ivov @tomi @janober