-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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: Prevent NodeApiError rewraping (no-changelog) #9627
fix: Prevent NodeApiError rewraping (no-changelog) #9627
Conversation
@@ -63,6 +63,7 @@ export async function activeCampaignApiRequest( | |||
return responseData[dataKey] as IDataObject; | |||
} | |||
} catch (error) { | |||
if (error instanceof NodeApiError) throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there must be a better way to do this instead of having to repeat this so many times.
perhaps we can try moving this check inside the NodeApiError
constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider this, but what if we actually want to re-wrap error? and if we decide to do that for NodeApiError
we probably want same policy for other error types
Despite a lot of files changed this would be less invasive and better for performance, as it's simple check
updated pr to disable errors re-wrapping
This reverts commit d70d041.
…do-not-re-wrap-error-in-nodes-api-request-function
…do-not-re-wrap-error-in-nodes-api-request-function
…do-not-re-wrap-error-in-nodes-api-request-function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last time we did this, it was done in the NodeError
, and that caused a lot of issues. I'm hoping that since these error classes aren't extended, this change doesn't cause those same issues 🤞🏽
2 flaky tests on run #5320 ↗︎
Details:
cypress/e2e/5-ndv.cy.ts • 2 flaky tests
Review all test suite changes for PR #9627 ↗︎ |
✅ All Cypress E2E specs passed |
Got released with |
Summary
Do not re-wrap error in node's API request function if it's already NodeApiError
Related tickets and issues
https://linear.app/n8n/issue/NODE-1392/do-not-re-wrap-error-in-nodes-api-request-function-if-its-already
https://linear.app/n8n/issue/NODE-1285/error-display-in-output-pane-improvements