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

[Spike] Use Express / Koa for HTTP processing #1071

Closed
dhmlau opened this issue Mar 1, 2018 · 11 comments
Closed

[Spike] Use Express / Koa for HTTP processing #1071

dhmlau opened this issue Mar 1, 2018 · 11 comments
Assignees
Labels
Milestone

Comments

@dhmlau
Copy link
Member

dhmlau commented Mar 1, 2018

See original text in #1038

To evaluate different approach:

  • see how much effort to improve our HTTP pipeline (our homegrown) to deal with common HTTP features/policies
  • pros and cons to leverage Express base without routing plus the common middleware, and Koa
  • if we can cherrypick the enhancement from Express/Koa, then we can reuse the middleware and no need to maintain the modules.

Coming from discussion with @raymondfeng

@dhmlau
Copy link
Member Author

dhmlau commented Mar 11, 2018

assigning this to @raymondfeng as he has a PR #1082 for this

@b-admike
Copy link
Contributor

b-admike commented Apr 10, 2018

From our spike result meeting today regarding #1082, here are some questions and tasks that we think follow from it:

  • Refactor more common interfaces into @loopback/http-server package from @loopback/rest (like low-level http response writer, parser etc.). In other words, from Raymond's comment, [Continue to] Refactor the http layer constructs and logic out of the @loopback/rest module for better reusability and cleaner separations.
  • Refactor testlab to two packages testlab and testlab-http (circular dependency will arise otherwise)
  • Get rid of http request and response dependency in sequence actions (rely on context completely) (https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/internal-types.ts)
  • Which web framework will we use? find out what is best: express, koa? or do we keep/improve our own homegrown module?
    • List supported use-cases for http processing; what do we need? CORS, compression, static file serving etc.
    • Look into maturity, availability of middleware, adoption, etc
  • How do we align the chosen framework's middleware pipeline with our sequence of actions?
  • Create a common http client for outbound connections (common http tier to do pre/post processing)
    @strongloop/lb-next-dev @bajtos Thoughts?

@bajtos
Copy link
Member

bajtos commented Apr 11, 2018

My thoughts:

Refactor more common interfaces into @loopback/http-server package from @loopback/rest (like low-level http response writer, parser etc.). In other words, from Raymond's comment, [Continue to] Refactor the http layer constructs and logic out of the @loopback/rest module for better reusability and cleaner separations.

This sounds reasonable 👍

Refactor testlab to two packages testlab and testlab-http (circular dependency will arise otherwise)

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 supertest, so that testlab-http becomes obsolete and people can use supertest directly. I am not sure if that's possible though!

Get rid of http request and response dependency in sequence actions (rely on context completely) (https://github.com/strongloop/loopback-next/blob/master/packages/rest/src/internal-types.ts)

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 app.handler((req, res, sequence) => {/* custom sequence*/}). Do we want to keep it around? Will it be possible to implement this API when request and response objects are stored in context only?

Which web framework will we use? find out what is best: express, koa? or do we keep/improve our own homegrown module?

+1

How do we align the chosen framework's middleware pipeline with our sequence of actions?

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.

Create a common http client for outbound connections (common http tier to do pre/post processing)

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.

@b-admike
Copy link
Contributor

Thank you for your feedback @bajtos. I think the best person to explain further for the concerns you raised is @raymondfeng.

@raymondfeng
Copy link
Contributor

Ideally, we should improve our REST package to make it more compatible with supertest, so that testlab-http becomes obsolete and people can use supertest directly. I am not sure if that's possible though!

In my PR, the HttpEndpoint already exposes url (bound) which can be used by supertest (request(endpoint.url)).

@raymondfeng
Copy link
Contributor

raymondfeng commented Apr 13, 2018

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 app.handler((req, res, sequence) => {/* custom sequence*/}). Do we want to keep it around? Will it be possible to implement this API when request and response objects are stored in context only?

Once we introduce a web framework like Express, we should find the divide of responsibility between the native middleware and our sequence of actions. I think our actions should be mostly handle concerns beyond the transport, such as how to route REST API calls. Lower level http req/res handling should be hooked into the web framework as much as possible. I'm open to being practical.

DI or not for the action composition is another discussion out of scope of this issue.

@hacksparrow
Copy link
Contributor

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 rest package. Since Express' request and response objects are basically Node's, working with them is easier and faster.

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.

@bajtos

@bajtos
Copy link
Member

bajtos commented Apr 26, 2018

I am ok with using Express, even though I disagree that porting @loopback/rest to Koa would require a lot of effort.

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

@hacksparrow
Copy link
Contributor

Thanks for the feedback Miroslav, will take all those into consideration in the implementation.

@dhmlau
Copy link
Member Author

dhmlau commented May 4, 2018

@raymondfeng, is this ticket good to close?

@dhmlau
Copy link
Member Author

dhmlau commented May 10, 2018

Closing it as the following tasks had created as a result of this spike:
#1290
#1291
#1292
#1293

@dhmlau dhmlau closed this as completed May 10, 2018
@shimks shimks added this to the April 2018 milestone May 22, 2018
@bajtos bajtos added the p1 label Aug 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants