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.

Sign up for GitHub

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

Jump to bottom

feat: webhooks #20129

Merged
merged 22 commits into from May 3, 2024
Merged

feat: webhooks #20129

merged 22 commits into from May 3, 2024

Conversation

Marc-Roig
Copy link
Contributor

@Marc-Roig Marc-Roig commented Apr 16, 2024 •
edited

What does it do?

Trigger webhooks on document service.

Only if using the document service, the webhooks will be triggered. So, using the query engine will not trigger them.

A schema of what triggers what:

image

@vercel Vercel
Copy link

vercel bot commented Apr 16, 2024 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 3, 2024 7:44am

@Marc-Roig Marc-Roig marked this pull request as ready for review April 17, 2024 07:58
@Marc-Roig
Copy link
Contributor Author

Marc-Roig commented Apr 17, 2024 •
edited

Don't want to clutter this PR so I will be adding the following improvements in other prs:

  • Publishing now triggers entry.publish & entry.delete (because we are deleting old published versiosn before deleting), there will be a pr to prevent that
  • Optimize the document service , we can save many queries that we are doing atm
  • Documentation
  • Tests

@Marc-Roig Marc-Roig self-assigned this Apr 17, 2024
@Marc-Roig Marc-Roig added pr: feature This PR adds a new feature source: core:core Source is core/core labels Apr 17, 2024
markkaylor
markkaylor approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@markkaylor markkaylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, did you want to me to test it out too?

@Marc-Roig
Copy link
Contributor Author

Marc-Roig commented Apr 17, 2024 •
edited

@markkaylor I will ask for QA in one of the following PRs where I improve a couple things
thank you for the review 🚀

remidej
remidej approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@remidej remidej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good! I just have one question if you can clarify

packages/core/core/src/services/document-service/webhooks.ts Outdated
Comment on lines 49 to 53
const emitWebhook = async (
uid: UID.Schema,
eventName: WebhookEvent,
entry: Modules.Documents.AnyDocument
) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I find odd about this function is that it's not webhook specific. What it technically does is just emit an event, which affects webhooks as much as audit logs or any potential other listener.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we should prevent any type of event being userd in this function? it could easily be an if case

@joshuaellis
Copy link
Member

Is this PR implementing stable webhook payloads, or is that later? 👀🥺

@alexandrebodin
Copy link
Member

Commenting before reviewing:

  • Should we emit document.xxx events and deprecate the entry. events just to keep some retro compat ?

@Marc-Roig
Copy link
Contributor Author

@alexandrebodin I am hesitant on the approach for keeping retrocompatibility,

Assuming we don't want to trigger both "document.xxx" & "entry.xxx" at the same time (e.g document.create && entry.create):

  • we can give users the ability which one they want triggered in the Webhooks Settings page (this option might require design)
  • we can add a flag in the config to trigger v4 webhooks, and we could even keep the same payload as before (without populating)

wdyt?

@Marc-Roig
Copy link
Contributor Author

@joshuaellis it's implementing stable payload except for delete() and unpublish() which we might need to find other solutions for those

@alexandrebodin
Copy link
Member

@alexandrebodin I am hesitant on the approach for keeping retrocompatibility,

Assuming we don't want to trigger both "document.xxx" & "entry.xxx" at the same time (e.g document.create && entry.create):

  • we can give users the ability which one they want triggered in the Webhooks Settings page (this option might require design)
  • we can add a flag in the config to trigger v4 webhooks, and we could even keep the same payload as before (without populating)

wdyt?

I think we want to trigger both. those are events or webhooks sent. up to the user to listen for them or not :)

@Marc-Roig
Copy link
Contributor Author

Alright :) will send them both

@Marc-Roig
feat: refactor publication methods to not trigger entry.delete webhook (
87c535a 
#20136)

* feat: refactor publication methods

* fix: remove unnecessary chain op in db query
alexandrebodin
alexandrebodin requested changes Apr 29, 2024
View reviewed changes
packages/core/core/src/services/event-hub.ts Outdated
@@ -92,7 +92,10 @@ export default function createEventHub(): EventHub {
},

removeAllListeners() {
return eventHub.destroy();
const destroy = eventHub.destroy();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum I wonder if we should replace this with just a listeners.clear & add a removeAllSubscribers. & make destroy do

this.removeAllListeners();
this.removeAllSubscribers();

wdyt ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:cheff-kiss: sounds great to me, will do that

@markkaylor markkaylor mentioned this pull request May 2, 2024
Open
28 tasks
@Marc-Roig Marc-Roig merged commit 23fa63e into v5/main May 3, 2024
78 of 83 checks passed
@Marc-Roig Marc-Roig deleted the v5/webhooks branch May 3, 2024 09:41
@verdverm
Copy link

verdverm commented May 3, 2024

merged, nice!

Any chance we can get a new beta with this? (despite releasing one about a day ago)

@Marc-Roig
Copy link
Contributor Author

@verdverm sorry missed this message! Probalby a bit late but just in case this was not released in the latest beta:
0.0.0-experimental.17b4116f461a49b8ce5386f7c8d79c511d40fb3b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexandrebodin alexandrebodin alexandrebodin approved these changes

@remidej remidej remidej approved these changes

@markkaylor markkaylor markkaylor approved these changes

@Feranchz Feranchz Awaiting requested review from Feranchz

Assignees

@Marc-Roig Marc-Roig

Labels
pr: feature This PR adds a new feature source: core:core Source is core/core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to add webhooks
7 participants
@Marc-Roig @joshuaellis @alexandrebodin @derrickmehaffy @verdverm @remidej @markkaylor