-
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
[RFC] feat: use express/koa/... as the http framework #1082
Conversation
4ca491a
to
6d81c54
Compare
I'll take a look when I get back from my vacation (not sooner than in two weeks). |
My main problem with Express is its concept of the So far, we based the design of our REST layer on the assumption that each sequence action (a middleware) always returns back to the framework, so that subsequent actions can be invoked even if we already have the response object. This is important for things like logging the amount of time needed to handle the request.
If we want to use a higher-level framework to provide HTTP server functionality for our REST layer, then I am proposing to use Koa instead of Express. AFAIK, Koa has better support for async/await style of programming and most importantly, it's designed in such way that middleware is executed even after the response object has been constructed. |
Few more high-level ideas and questions to consider: 1] It's already possible to mount a LB4 REST server on any Express application: const lbApp = new RestApplication();
const expressApp = express();
expressApp.use('/api', lbApp.getSync<RestServer>('servers.RestServer').handleHttp); If/when we land #1084, the example will become even shorter: const lbApp = new RestApplication();
const expressApp = express();
expressApp.use('/api', lbApp.handleHttp); 2] How do you envision the interaction of our Sequence with Express/Koa middleware?
|
@bajtos Thank you for the feedback. Let me clarify my view:
The req/res processing pipeline will look like:
On topics of Express vs. Koa, I have debated with myself too.
|
d21ecba
to
3ec9a9f
Compare
21afc99
to
d2e1197
Compare
@raymondfeng I see your point.
AFAIK, there is no response chain in Express and that's my concern! Re-posting my earlier question - what's you answer please? Who is going to be responsible for handling unknown request paths (404 Not Found) and for general error handling? Should LB4 Sequence behave like a good Express middleware and invoke next for both unknown paths and errors? How are we going to capture this in our Sequence design?
I agree with you that Express beats Koa in adoption, maturity and compatibility. OTOH, the same thing can be said about callbacks - they beat Promises and async/await in adoption and maturity too. And yet when starting LoopBack Next, we decided to throw callbacks away and use async/await everywhere. Shouldn't we make the same bold move here, say Express good bye and focus only on integration with frameworks that are designed for async/await? Anyhow, as long as we are able to find a solid solution for integrating our sequence-action based request processing with next-callback-based middleware style of Express, then I am find with supporting Express too.
I am confused. At one hand, you are saying that Express middleware should be only the last resort, but then you are mentioning Passport, which is IMO not something to add to LoopBack as the last resort solution. Are you expecting people to use Passport with Express middleware? If so, then how do you envision integration with LoopBack4 - e.g. how to access the current user set by Passport via LB4 dependency injection? If not, then what kind of middleware do you expect people to use in LB4+Express applications? I'd like to better understand the use cases you have in mind, so that we can implement a solution that's well tailored to those use cases. |
Let me clarify. I see a high-level Node.js http framework such as
My view to embrace one of the frameworks is:
|
There are also a few objectives that I want to achieve:
|
7452154
to
f0eaf37
Compare
06aeced
to
d23de04
Compare
debug('Finishing request: %s', request.originalUrl); | ||
next(); | ||
}) | ||
.catch(err => next(err)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When next()
called on L53 throws an error, this error is converted to a rejected promise, which is passed via next(err)
. As a result, the next
callback is called twice.
Here is a better version:
handler(httpCtx).then(
() => {
debug('Finishing request: %s', request.originalUrl);
next();
},
next);
res: response, | ||
request, | ||
response, | ||
next, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to expose next
as a public property of HttpContext that's available to all users. At minimum, we should add some sort of a guard that will throw a helpful error if/when next
is called multiple times for the same request.
implements HttpFactory<Request, Response, HttpApplication> { | ||
createEndpoint(config: HttpServerConfig, handler: HttpHandler) { | ||
// Create an express representing the server endpoint | ||
const app = express() as HttpApplication; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, I'd like the factory function express()
to be configurable, so that users can customize the express instance and/or configuration. More importantly, I'd like users to be able to choose a different express-compatible server implementation like https://www.fastify.io
if (err) throw err; | ||
}); | ||
const request = Object.setPrototypeOf(req, expressRequest); | ||
const response = Object.setPrototypeOf(res, expressResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning: Changing the [[Prototype]] of an object is, by the nature of how modern JavaScript engines optimize property accesses, a very slow operation, in every browser and JavaScript engine. The effects on performance of altering inheritance are subtle and far-flung, and are not limited to simply the time spent in Object.setPrototypeOf(...) statement, but may extend to any code that has access to any object whose [[Prototype]] has been altered. If you care about performance you should avoid setting the [[Prototype]] of an object. Instead, create a new object with the desired [[Prototype]] using Object.create().
Can we find a different solution please, one that uses idiomatic JavaScript/TypeScript?
Also, I don't understand why is it necessary to add express prototype to the req/res objects, when createHttpContext
is called from toMiddleware
that already receives req/res objects from express, therefore these objects already have express additions.
requestContext.bind(RestBindings.Http.REQUEST).to(req); | ||
requestContext.bind(RestBindings.Http.RESPONSE).to(res); | ||
requestContext.bind(RestBindings.Http.REQUEST).to(httpCtx.request); | ||
requestContext.bind(RestBindings.Http.RESPONSE).to(httpCtx.response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should replace request/response bindings with a single binding holding the full HTTP context. Thoughts?
@raymondfeng As I see it, this pull request contains multiple changes that could be made independently:
Could you please extract the first change, introduction of
Is this still needed, considering the discussion in #1255? IIUC, Koa and Express are so incompatible that it's not possible (or at least too cumbersome) to support both. In that light, can we simplify the changes you are proposing here and always assume both Request and Response objects are coming from Express, with all helper methods and parsed request metadata already attached to it? One more thing to consider - the current implementation on const loopBackApp = new RestApplication();
const expressApp = express();
express.use(loopBackApp. requestHandler); IMO opinion, this is much simpler than what you are proposing in this pull request and fulfills most of what we need (at least for DP3). The only change that may be needed is to extend
I think this is reasonably independent of the other two changes so maybe the work on this can start in parallel with item 1 (in its own dedicated pull request)? |
@bajtos all points noted. I will be working on this PR and the related tasks. |
@raymondfeng I am proposing to close this pull request in favor of newer ones. Any objections? |
@raymondfeng @hacksparrow , i think we've extracted some code from this PR and created another PR (and merged). Can we close this PR? |
Sure |
This PR might be a bit controversial.
Connect to #1071.
Key incentives:
Introduce
HttpContext
object to wrap request/response of some flavors (such as Node core http, Express, Koa, or our home grown). In this PR, we directly use Express Request/Response as the http interfaces.Extract the logic to create (REST) servers into
http-server.ts
and provide a reference implementation using Express (only the http core, not routing) - it should be possible to refactor the common HTTP logic into a separate module (@loopback/http-server
) so that similar stacks such as Koa, Hapi, or Restify can be potentially plugged in.Pros:
Cons:
Checklist
npm test
passes on your machinepackages/cli
were updatedpackages/example-*
were updated