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.

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

Prehandler support #75

Merged
merged 10 commits into from
Mar 11, 2024
Merged

Prehandler support #75

merged 10 commits into from
Mar 11, 2024

Conversation

kibertoad
Copy link
Owner

No description provided.

@kibertoad kibertoad marked this pull request as ready for review March 10, 2024 13:41
@kibertoad
Copy link
Owner Author

@dariacm @CarlosGamero This PR was two months in the making, I don't want to delay it any further, so I'll tackle the documentation in a separate PR; it's been high time we've started writing dialect-specific documentation too, so I'll incorporate it into the larger docs rework.

Comment on lines +81 to +82
const prehandlerOutput = await this.processPrehandlers(message, messageType)
const barrierResult = await this.preHandlerBarrier(message, messageType, prehandlerOutput)
Copy link
Collaborator

Choose a reason for hiding this comment

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

No part of this PR.
If the barrier doesn't allow the message to be consumed we will execute preHandlers again, so maybe as a future improvement do you think that could make sense to somehow store preHandlers results and avoid executing them again?

To be clear, I think it is fine and I am not suggesting doing it now, just thinking about the possibility

Copy link
Owner Author

Choose a reason for hiding this comment

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

It would be great, but I doubt a fully universal solution is possible here. Imagine a prehandler with permission checks - if permissions changed since the last time we checked, prehandler results may be different.

Caching downstream on a case-by-case basis (e. g. how we cache user data in user plugin now) looks like a more feasible approach.

Copy link
Collaborator

@CarlosGamero CarlosGamero Mar 11, 2024

Choose a reason for hiding this comment

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

I see, a fully universal solution doesn't seem to be the right approach you are right, so yeah, we would need to go with a case-by-case one 💯 but that is something for future improvement :D

Comment on lines +65 to +67
// MonoSchema support is going away in the next semver major, so we don't care about coverage strongly
/* c8 ignore start */
// eslint-disable-next-line max-params
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! removing mono consumers will help to speed up development on the library 🙌, thanks Igor 🙇

Copy link
Collaborator

@CarlosGamero CarlosGamero left a comment

Choose a reason for hiding this comment

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

Impressive work 🚀! thank you so much 🙌

@kibertoad kibertoad merged commit 642b538 into main Mar 11, 2024
5 checks passed
@kibertoad kibertoad deleted the feat/prehandlers branch March 11, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants