-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Spike] Use Express / Koa for HTTP processing #1071
Comments
assigning this to @raymondfeng as he has a PR #1082 for this |
From our spike result meeting today regarding #1082, here are some questions and tasks that we think follow from it:
|
My thoughts:
This sounds reasonable 👍
I need more details to be able to understand this point and provide feedback. Ideally, we should improve our REST package to make it more compatible with
Hmm, this deserves deeper consideration. What are the reasoning for this change? From what I remember, the current Sequence is intentionally designed to be independent of/decoupled from DI/IoC. Ideally, people should not be forced into our IoC to write a custom Sequence, but I am ok to revisit this idea. The second use case to consider is the short-hand
+1
I'd like to specifically see how we will approach error handling. When mounting a LB4 app on express/koa/etc., users should have the possibility to decide whether they want to handle errors inside LB4 or use the app-wide express/koa/etc.-level error handler for all endpoints.
IMO, http client and outbound connections should be left out of scope of the work on hardening/improving our HTTP server (inbound connections) and moved to the story/epic covering integration with REST/SOAP services - see #1036. |
Thank you for your feedback @bajtos. I think the best person to explain further for the concerns you raised is @raymondfeng. |
In my PR, the HttpEndpoint already exposes |
Once we introduce a web framework like DI or not for the action composition is another discussion out of scope of this issue. |
From yesterday's spike discussion we have decided to go ahead with Express. There's no doubt, there are some good aspects about Koa, but we are choosing Express over Koa at this stage mainly because: It will take a good amount of effort to port Koa's request and response objects to work with the LB4 Having said that, we are open to re-considering the use of Koa as our underlying HTTP framework at a later time. As for any challenges and requirements by the use of Express, we will address them along the way. |
I am ok with using Express, even though I disagree that porting Cross-posting from #1255 (comment): I posted few thoughts and review comments for Raymond's RFC pull request, start reading here: #1082 (review) To keep our code base simple, I am proposing to assume that LB4 applications (REST servers) will be always mounted as an Express middleware and use request/response types from Express everywhere. Let's not complicate our code by supporting different request/response flavours, when most (if not all) of our users are going to follow our guidance and use LB4 with Express. The only thing I'd like to keep configurable is the express factory function. I'd like LB4 users to be able to switch from Express to a compatible framework, e.g. https://www.fastify.io or perhaps something based on https://github.com/mafintosh/turbo-http |
Thanks for the feedback Miroslav, will take all those into consideration in the implementation. |
@raymondfeng, is this ticket good to close? |
See original text in #1038
To evaluate different approach:
Coming from discussion with @raymondfeng
The text was updated successfully, but these errors were encountered: