feat(framework, web, application-generic): Propagate Bridge server errors to Bridge client #6726
Conversation
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -64,6 +64,7 @@ export function ErrorPrettyRender({ error: unparsedError }) { | |||
| {isExpanded && ( | |||
| <pre | |||
| className={css({ | |||
| whiteSpace: 'pre-wrap', | |||
There was a problem hiding this comment.
Preserve new line characters from the stack trace
| @@ -74,7 +75,8 @@ export function ErrorPrettyRender({ error: unparsedError }) { | |||
| fontFamily: 'mono', | |||
| })} | |||
| > | |||
| {JSON.stringify(error.data, null, 2)} | |||
| {error.data?.stack} | |||
| {!error.data?.stack && JSON.stringify(error.data, null, 2)} | |||
There was a problem hiding this comment.
A little bit yucky here, but we can tidy this up when we rebuild Local Studio to use proper type-guards.
| @@ -231,7 +240,7 @@ export class ExecuteBridgeJob { | |||
| statusCode: response.statusCode, | |||
| retryCount: response.retryCount, | |||
| message: response.statusMessage, | |||
| ...(body && body?.length > 0 ? { raw: JSON.parse(body) } : {}), | |||
| ...(body && body?.length > 0 ? { raw: rawMessage } : {}), | |||
There was a problem hiding this comment.
This was another case where we weren't safely handling the parsing of the response body, which can lead to no activity feeds being logged at all due to unhandled ParseError
| ) { | ||
| // Handle known Bridge errors. Propagate the error code and message. | ||
| if (error instanceof HTTPError && isFrameworkError(body)) { | ||
| // Handle known Framework errors. Propagate the error code and message. | ||
| throw new HttpException(body, error.response.statusCode); |
There was a problem hiding this comment.
Using the exported type-guard now 🚀
| "build:watch": "tsup --watch", | ||
| "$comment:bump:prerelease": "This is a workaround to support `npm version prerelease` with lerna", | ||
| "bump:prerelease": "npm version prerelease --preid=alpha & PID=$!; (sleep 1 && kill -9 $PID) & wait $PID", | ||
| "release:alpha": "pnpm bump:prerelease || pnpm build && npm publish", | ||
| "devtool": "tsx ./scripts/devtool.ts" | ||
| "devtool": "tsx ./scripts/devtool.ts", | ||
| "check:circulars": "madge --circular --extensions ts --exclude ../../shared ./src" |
There was a problem hiding this comment.
madge is a neat tool which helps us to find circular dependencies. Typescript will actually try to resolve circular dependencies, but they are bad in public packages as they can cause the TS-Compiler to choke when building the AST. This impacts DX both internally and externally.
We should probably add madge circular dep checks to all our public packages. cc @LetItRock
| new ProviderExecutionFailedError('sendgrid', PostActionEnum.EXECUTE, new Error('Preview execution failed')) | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Adding some explicit tests to ensure that execution errors are reported correctly, allowing the original Framework error to bubble up.
| @@ -272,7 +274,7 @@ export class Client { | |||
| return async (stepId, stepResolve, options) => { | |||
| const step = this.getStep(event.workflowId, stepId); | |||
| const controls = await this.createStepControls(step, event); | |||
| const isPreview = event.action === 'preview'; | |||
| const isPreview = event.action === PostActionEnum.PREVIEW; | |||
There was a problem hiding this comment.
A bunch of fixes to use the enum instead of hardcoded values.
| @@ -356,14 +358,10 @@ export class Client { | |||
| const actionMessages = { | |||
| [PostActionEnum.EXECUTE]: 'Executing', | |||
| [PostActionEnum.PREVIEW]: 'Previewing', | |||
| }; | |||
| } as const; | |||
There was a problem hiding this comment.
Simplifying the error message lookup map.
| throw new ProviderExecutionFailedError( | ||
| `Failed to execute provider: '${provider.type}'.\n${(error as Error).message}` | ||
| ); | ||
| throw new ProviderExecutionFailedError(provider.type, event.action, error); |
There was a problem hiding this comment.
Using the new, DRY error interface for execution errors.
| throw error; | ||
| } else { | ||
| throw new StepExecutionFailedError(step.stepId, event.action, error); | ||
| } |
There was a problem hiding this comment.
Let the original error bubble up. We choose not to encapsulate the isFrameworkError check in the StepExecutionFailedError constructor logic because doing so necessitates putting it behind the new keyword, which breaks the stack-trace of the original error.
| throw error; | ||
| } else { | ||
| throw new StepExecutionFailedError(step.stepId, event.action, error); | ||
| } |
There was a problem hiding this comment.
Same here, letting the original error bubble up, but for step preview.
| STEP_EXECUTION_FAILED_ERROR = 'StepExecutionFailedError', | ||
| STEP_NOT_FOUND_ERROR = 'StepNotFoundError', | ||
| WORKFLOW_ALREADY_EXISTS_ERROR = 'WorkflowAlreadyExistsError', | ||
| WORKFLOW_NOT_FOUND_ERROR = 'WorkflowNotFoundError', | ||
| WORKFLOW_PAYLOAD_INVALID_ERROR = 'WorkflowPayloadInvalidError', | ||
| } |
There was a problem hiding this comment.
Just sorting the error codes for readability.
| import { version } from '../../package.json'; | ||
|
|
||
| export const SDK_VERSION = version; | ||
| export { version as SDK_VERSION } from '../../package.json'; |
There was a problem hiding this comment.
Simpler export, save some bytes.
| stack: message, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
A base server error that encapsulates handling of stack-traces onto the data object.
| this.statusCode = statusCode; | ||
| this.code = code as ErrorCodeEnum; // TODO: Throw known error codes from Platform. | ||
| } | ||
| } |
There was a problem hiding this comment.
Moved to platform.errors.ts, it's not an execution error.
| @@ -1,5 +1,5 @@ | |||
| import { ErrorCodeEnum, HttpMethodEnum, HttpStatusEnum } from '../constants'; | |||
| import { enumToPrettyString } from '../utils'; | |||
| import { enumToPrettyString } from '../utils/string.utils'; | |||
There was a problem hiding this comment.
Import from the deeper module to avoid a circular dependency.
| @@ -7,3 +8,4 @@ export * from './resource.errors'; | |||
| export * from './signature.errors'; | |||
| export * from './step.errors'; | |||
| export * from './workflow.errors'; | |||
| export { isFrameworkError } from './guard.errors'; | |||
There was a problem hiding this comment.
We want to expose isFrameworkError for Bridge API clients to handle their errors neatly with error determination handled by the Framework package.
| } | ||
| } | ||
|
|
||
| export abstract class ResourcePreviewFailed extends InternalServerError { |
There was a problem hiding this comment.
We don't need a separate class for preview errors, we just use the Execution errors abstract class with different action injected.
| @@ -27,6 +26,7 @@ import { | |||
| } from './errors'; | |||
| import type { Awaitable, EventTriggerParams, Workflow } from './types'; | |||
| import { initApiClient } from './utils'; | |||
| import { isPlatformError } from './errors/guard.errors'; | |||
There was a problem hiding this comment.
Different export here because isPlatformError isn't part of the public package API.
| @@ -1,5 +1,6 @@ | |||
| export { Client } from './client'; | |||
| export * from './constants'; | |||
| export * from './errors'; | |||
There was a problem hiding this comment.
We want to expose all errors to the public API because they can be used to build exhaustive error handling checks.
| @@ -1,5 +1,5 @@ | |||
| import { ChatProviderIdEnum } from '@novu/shared'; | |||
| import { Schema } from '../../../types'; | |||
| import { Schema } from '../../../types/schema.types'; | |||
There was a problem hiding this comment.
More updates here and below to fix a circular dependency loop.
| ? { | ||
| [P in keyof T]-?: DeepRequired<T[P]>; | ||
| } | ||
| : T; |
There was a problem hiding this comment.
Used by the ServerError constructor to make all the Error constructor arguments required.
| @@ -22,7 +22,7 @@ export const initApiClient = (apiKey: string, baseURL = 'https://api.novu.co') = | |||
| } else if (checkIsResponseError(resJson)) { | |||
| throw new PlatformError(resJson.statusCode, resJson.error, resJson.message); | |||
| } else { | |||
| throw new BridgeError('Error processing API request to Novu Cloud from Bridge application.'); | |||
| throw new BridgeError(resJson); | |||
There was a problem hiding this comment.
Propagate the actual error to the API client to stop obfuscating the actual error.
| * such as errors due to unsupported runtime environments. | ||
| */ | ||
| export class BridgeError extends ServerError { | ||
| statusCode = HttpStatusEnum.INTERNAL_SERVER_ERROR; |
There was a problem hiding this comment.
Using a 500 error code here because it's an unknown exception.
| constructor(resource: ResourceEnum, id: string) { | ||
| super(`Failed to preview ${toPascalCase(resource)} with id: \`${id}\`. Please try again later.`); | ||
| export abstract class ResourceExecutionFailed extends ServerError { | ||
| statusCode = HttpStatusEnum.BAD_GATEWAY; |
There was a problem hiding this comment.
Using a 502 error code here as I think it's the best fit - we know an error occurred during execution, and the Framework was acting as a "gateway" to the logic built by the implementor. By throwing a non-500 error code, all the Novu Platform exception filters won't treat it as unhandled (we only treat 500 errors as exceptions)
novu
@novu/client
@novu/framework
@novu/headless
@novu/nest
@novu/nextjs
@novu/js
@novu/notification-center
@novu/node
@novu/providers
@novu/react
@novu/react-native
@novu/shared
@novu/stateless
commit: |
What changed? Why was the change needed?
isFrameworkErrortype-guard for use by bridge clientsisFrameworkErrorguard inExecuteBridgeRequestuse-case to separate client error handling concernsScreenshots
Bridge error in local Bridge app logs
Bridge error in Studio activity feed
Bridge error in Studio preview
Unknown Bridge error in Sentry
Bridge error in Dashboard preview
Bridge error in Dashboard activity feed
Expand for optional sections
Related enterprise PR
Special notes for your reviewer