fix(Wait Node): Authentication fix #10236
Conversation
michael-radency
added
node/improvement
| @@ -294,6 +302,29 @@ export class Wait extends Webhook { | |||
| default: 'timeInterval', | |||
| description: 'Determines the waiting mode to use before the workflow continues', | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
there seems to be a bit of duplication between this and the next property. is this because the auth options are more limited here?
There was a problem hiding this comment.
yes, we probably should avoid sharing specialized node's properties between different node types as this creates some coupling
| @@ -140,8 +140,15 @@ const checkResponseModeConfiguration = (context: IWebhookFunctions) => { | |||
| } | |||
| }; | |||
|
|
|||
| const authProperty = (nodeType: string) => { | |||
| if (nodeType === 'n8n-nodes-base.wait') { | |||
| return 'incomingAuthentication'; | |||
There was a problem hiding this comment.
since this is defined in the node already, could we somehow pick it from there instead ?
| const nodeVersion = context.getNode().typeVersion; | ||
| export async function formWebhook( | ||
| context: IWebhookFunctions, | ||
| authProperty = FORM_TRIGGER_AUTHENTICATION_PROPERTY, |
There was a problem hiding this comment.
This is why I brought up this conversation. If we switch to using class inheritance (instead of the custom style we use), we wouldn't need to pass data around like this, and we could use this.authPropertyName, like we do in Webhook and Wait nodes.
| displayName: 'Authentication', | ||
| name: 'incomingAuthentication', | ||
| type: 'options', | ||
| options: [ | ||
| { | ||
| name: 'Basic Auth', | ||
| value: 'basicAuth', | ||
| }, | ||
| { | ||
| name: 'None', | ||
| value: 'none', | ||
| }, | ||
| ], | ||
| default: 'none', |
There was a problem hiding this comment.
we are still duplicating a lot, and the style of this block isn't consistent with the one after this.
The issue with duplication like this is that every time we need to update something in one place, we need to update it in all the copies. And we don't always do that, so we end up with slightly different versions of the same code that later someone else has to clean up.
There was a problem hiding this comment.
I would argue that here it is justifiable as Wait node basically include both Form and Webhook node, and though they have similar properties they are completely different entity, trying too mach reuse code we are tying them together so changes in Webhook would require Form change, which is not what we want
Ideally we want some library of common properties and helpers to setup them, like returnAllOrLimit and helpers to populate options and fixedCollection
There was a problem hiding this comment.
What we need is a better way to describe properties, that's type-safe, and allows code reuse without being unintuitive.
Right now we are battling between duplicating code or making code unreadable, and neither is great.
|
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
|
✅ All Cypress E2E specs passed |
|
Got released with |
Summary
Authentication for Wait node
Hint to warn about specific of manual execution
Related Linear tickets, Github issues, and Community forum posts
https://linear.app/n8n/issue/NODE-1536/wait-node-authentication