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

Upgrade versions and used Handler interface #193

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Upgrade versions and used Handler interface #193

wants to merge 1 commit into from

Conversation

pmlopes
Copy link

@pmlopes pmlopes commented Jan 29, 2020

Signed-off-by: Paulo Lopes [email protected]

The template was using an old version of vert.x and the handler should not implement the BodyHandler interface as it does not mean that you will get the HTTP body parsed. This needs to be passed on the bootstrap project and implement the vert.x Handler functional interface.

As a benefit this makes the function code smaller.

Still the original template or this PR do not expose the router to the function as a setup step. This would be interesting to chain middleware. As an example we could process CORS, or security before the function is executed.

Also post processing is not possible, for example a catch all error handler.

@alexellis
Copy link
Member

Hi @pmlopes thank you for this PR, please could you rebase?

@jconlon
Copy link

jconlon commented Jan 27, 2021

@pmlopes Like the idea of exposing the router. But Vert.x is now at 4.0.0. Are there reasons we dont want to use the latest 4.0.0?

@pmlopes
Copy link
Author

pmlopes commented Jan 28, 2021

@alexellis @jconlon This pr got lost on my side. Yes, we should upgrade it to 4.0.0. One of the improvements, application wize there should be almost to none code changes for the end user.

We could change the style from callback to future/promise style which improves readability and internally guarantees that the thread affinity is always correct as the future object will enforce it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants