Should we re-introduce a local event type for the webhooks? #530

Closed
opened 2026-02-04 20:13:43 +03:00 by OVERLORD · 2 comments
Owner

Originally created by @HannesOberreiter on GitHub (Jun 17, 2024).

Is this a feature for the backend or frontend?

Backend

What would you like?

Hi,

I noticed with the latest changes that there isn't an easy or programmatic way to get a list of all events sent to the webhook(s). Should we consider reintroducing a the variable for this purpose?
Currently the JSDoc is broken to automatically generate a type file, due to missing reference.

* @param {string} event - The event (see {@link Events}).)

Why is this needed?

It would greatly enhance the usability of the webhook without needing to scan the entire project. Additionally, in the future, we could automatically generate types from it as I would to for my webhook.

Other information

Additionally the change made it now more difficult to figure out the structure of the payload, which was already before a big hurdle if one is not familiar with the database models. Therefore, I would additionally suggest to further split of the payload on the webhooks utility so one can more easily read a possible payload by reading the javascript module.

(Little bit more Off-Topic, I see you leave async events in the event loop. I'm not familiar with the sails framework, is there a special way that the response is send outside of the event loop? As the current way to handle async events inside the requests without knowing if it resolves, fails or terminates seems rather risky for leaks and multiple responses)

Cheers,
Hannes

Originally created by @HannesOberreiter on GitHub (Jun 17, 2024). ### Is this a feature for the backend or frontend? Backend ### What would you like? Hi, I noticed with the latest changes that there isn't an easy or programmatic way to get a list of all events sent to the webhook(s). Should we consider reintroducing a the variable for this purpose? Currently the JSDoc is broken to automatically generate a type file, due to missing reference. ``` * @param {string} event - The event (see {@link Events}).) ``` ### Why is this needed? It would greatly enhance the usability of the webhook without needing to scan the entire project. Additionally, in the future, we could automatically generate types from it as I would to for my webhook. ### Other information Additionally the change made it now more difficult to figure out the structure of the payload, which was already before a big hurdle if one is not familiar with the database models. Therefore, I would additionally suggest to further split of the payload on the webhooks utility so one can more easily read a possible payload by reading the javascript module. (Little bit more Off-Topic, I see you leave async events in the event loop. I'm not familiar with the sails framework, is there a special way that the response is send outside of the event loop? As the current way to handle async events inside the requests without knowing if it resolves, fails or terminates seems rather risky for leaks and multiple responses) Cheers, Hannes
Author
Owner

@HannesOberreiter commented on GitHub (Jun 17, 2024):

I'm currently implementing the new version of the webhook on our dev server. I have a couple of concerns regarding the use of JSON objects as environment variables and would appreciate some clarification:

  • I'm not a fan of using JSON objects for environment variables due to the complications with escaping. Additionally, I don't believe it improves readability, as I still need to stringify the secrets in a JSON format within my secret manager.
  • Why does the webhook need to be an array? For those who are deeply involved in implementing their own webhook, managing such behavior can be done within the webhook itself. In my view, this requirement adds unnecessary configuration overhead that could be handled more efficiently within the webhook.

Could you please provide the reasoning behind these design choices?

I would it keep as simple as possible to and split it up again by separating again WEBHOOK_URL, WEBHOOK_BEARER, WEBHOOK_EVENTS_INCLUDE, WEBHOOK_EVENTS_EXCLUDE

Cheers,
Hannes

@HannesOberreiter commented on GitHub (Jun 17, 2024): I'm currently implementing the new version of the webhook on our dev server. I have a couple of concerns regarding the use of JSON objects as environment variables and would appreciate some clarification: - I'm not a fan of using JSON objects for environment variables due to the complications with escaping. Additionally, I don't believe it improves readability, as I still need to stringify the secrets in a JSON format within my secret manager. - Why does the webhook need to be an array? For those who are deeply involved in implementing their own webhook, managing such behavior can be done within the webhook itself. In my view, this requirement adds unnecessary configuration overhead that could be handled more efficiently within the webhook. Could you please provide the reasoning behind these design choices? I would it keep as simple as possible to and split it up again by separating again WEBHOOK_URL, WEBHOOK_BEARER, WEBHOOK_EVENTS_INCLUDE, WEBHOOK_EVENTS_EXCLUDE Cheers, Hannes
Author
Owner

@meltyshev commented on GitHub (Jun 18, 2024):

Hi!

Yes, you are right that it's better to add back the available event types 👍 We have plans to replace the sails framework sometime in the future and then it'll be possible to import these constants and use them instead of strings.

Regarding JSON, we thought it'd be nice to be able to configure multiple webhooks for different events (for example for notifications you could keep only notification* events) and that's just the first thing that came to mind as a solution. Also in the second version we want to bring most of the settings to the admin window, so there should no longer be a problem with using JSON in environment variables. But it's really not very nice and maybe we should revert it back to what you suggested, need to think about it...

UPD: big thanks for all the fixes and your PRs 🙏

@meltyshev commented on GitHub (Jun 18, 2024): Hi! Yes, you are right that it's better to add back the available event types 👍 We have plans to replace the sails framework sometime in the future and then it'll be possible to import these constants and use them instead of strings. Regarding JSON, we thought it'd be nice to be able to configure multiple webhooks for different events (for example for notifications you could keep only notification* events) and that's just the first thing that came to mind as a solution. Also in the second version we want to bring most of the settings to the admin window, so there should no longer be a problem with using JSON in environment variables. But it's really not very nice and maybe we should revert it back to what you suggested, need to think about it... UPD: big thanks for all the fixes and your PRs 🙏
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/planka#530