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.

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

fix(framework): Explicitly exit workflow evaluation early after evaluating specified stepId #6808

Merged

Conversation

rifont
Copy link
Contributor

@rifont rifont commented Oct 30, 2024

What changed? Why was the change needed?

  • For workflows with 2 or more steps, internal framework client code was being executed after the executing the specified stepId which resulted in a misleading log of Failed to hydrate stepId. This change adds an explicit check (a Promise cancellation token) to exit step evaluation early when the workflow is expected to conclude, preventing any further code execution including the misleading log. This approach is necessary whilst Node.js doesn't provide a native Promise cancellation API (which is a planned feature).
  • chore: remove ora dependency which provided a nice local Studio DX, but ultimately doesn't provide enough value for the size of an extra package. It's dependencies may also be incompatible with some serverless environments like Cloudflare. It also simplifies the test assertion.

Screenshots

BEFORE - the misleading Failed to hydrate stepId log is present

 POST /api/novu?action=trigger&workflowId=in-app-wf 200 in 343ms

Executing workflowId: 'in-app-wf'
 ✔ Executed stepId: `custom-step`
✔ Executed workflowId: `in-app-wf`

  ✔ Executed workflowId: 'in-app-wf'
  ├ σ stepId: 'custom-step'
  ├ α action: 'execute'
  └ Δ duration: '23.15ms'

 POST /api/novu?action=execute&workflowId=in-app-wf&stepId=custom-step 200 in 27ms
 ✗ Failed to hydrate stepId: `inbox` # 👈❌ MISLEADING LOG HERE ❌👈

Executing workflowId: 'in-app-wf'
 → Hydrated stepId: `custom-step`
 ✔ Executed stepId: `inbox`
✔ Executed workflowId: `in-app-wf`

  ✔ Executed workflowId: 'in-app-wf'
  ├ σ stepId: 'inbox'
  ├ α action: 'execute'
  └ Δ duration: '8.95ms'

 POST /api/novu?action=execute&workflowId=in-app-wf&stepId=inbox 200 in 13ms

AFTER - no misleading logs present

 POST /api/novu?action=trigger&workflowId=in-app-wf 200 in 290ms

Executing workflowId: 'in-app-wf'
  ✔ Executed stepId: `custom-step`
✔ Executed workflowId: `in-app-wf`

  ✔ Executed workflowId: 'in-app-wf'
  ├ σ stepId: 'custom-step'
  ├ α action: 'execute'
  └ Δ duration: '24.18ms'

 POST /api/novu?action=execute&workflowId=in-app-wf&stepId=custom-step 200 in 29ms

Executing workflowId: 'in-app-wf'
  → Hydrated stepId: `custom-step`
  ✔ Executed stepId: `inbox`
✔ Executed workflowId: `in-app-wf`

  ✔ Executed workflowId: 'in-app-wf'
  ├ σ stepId: 'inbox'
  ├ α action: 'execute'
  └ Δ duration: '31.45ms'

 POST /api/novu?action=execute&workflowId=in-app-wf&stepId=inbox 200 in 46ms
Expand for optional sections

Related enterprise PR

Special notes for your reviewer

Copy link

linear bot commented Oct 30, 2024

Copy link

netlify bot commented Oct 30, 2024

Deploy Preview for novu-stg-vite-dashboard-poc ready!

Name Link
🔨 Latest commit 38704e4
🔍 Latest deploy log https://app.netlify.com/sites/novu-stg-vite-dashboard-poc/deploys/6722888164693d0009ae8014
😎 Deploy Preview https://deploy-preview-6808--novu-stg-vite-dashboard-poc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rifont rifont changed the title Nv 4506 failed to hydrate stepid error with custom step and topic fix(framework): Explicitly exit workflow evaluation early after provided stepId Oct 30, 2024
@rifont rifont requested review from djabarovgeorge and SokratisVidros and removed request for djabarovgeorge October 30, 2024 19:23
@rifont rifont changed the title fix(framework): Explicitly exit workflow evaluation early after provided stepId fix(framework): Explicitly exit workflow evaluation early after evaluating provided stepId Oct 30, 2024
@rifont rifont changed the title fix(framework): Explicitly exit workflow evaluation early after evaluating provided stepId fix(framework): Explicitly exit workflow evaluation early after evaluating specified stepId Oct 30, 2024
Copy link

pkg-pr-new bot commented Oct 30, 2024

Open in Stackblitz

@novu/client

pnpm add https://pkg.pr.new/novuhq/novu/@novu/client@6808

@novu/framework

pnpm add https://pkg.pr.new/novuhq/novu/@novu/framework@6808

@novu/js

pnpm add https://pkg.pr.new/novuhq/novu/@novu/js@6808

@novu/headless

pnpm add https://pkg.pr.new/novuhq/novu/@novu/headless@6808

@novu/nest

pnpm add https://pkg.pr.new/novuhq/novu/@novu/nest@6808

@novu/nextjs

pnpm add https://pkg.pr.new/novuhq/novu/@novu/nextjs@6808

@novu/node

pnpm add https://pkg.pr.new/novuhq/novu/@novu/node@6808

@novu/notification-center

pnpm add https://pkg.pr.new/novuhq/novu/@novu/notification-center@6808

novu

pnpm add https://pkg.pr.new/novuhq/novu@6808

@novu/providers

pnpm add https://pkg.pr.new/novuhq/novu/@novu/providers@6808

@novu/react

pnpm add https://pkg.pr.new/novuhq/novu/@novu/react@6808

@novu/react-native

pnpm add https://pkg.pr.new/novuhq/novu/@novu/react-native@6808

@novu/shared

pnpm add https://pkg.pr.new/novuhq/novu/@novu/shared@6808

@novu/stateless

pnpm add https://pkg.pr.new/novuhq/novu/@novu/stateless@6808

commit: 38704e4

@rifont rifont merged commit 2bdf840 into next Oct 31, 2024
49 checks passed
@rifont rifont deleted the nv-4506-failed-to-hydrate-stepid-error-with-custom-step-and-topic branch October 31, 2024 14:00
LetItRock pushed a commit that referenced this pull request Nov 1, 2024
LetItRock pushed a commit that referenced this pull request Nov 1, 2024
LetItRock pushed a commit that referenced this pull request Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants