-
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
Decouple SwaggerRouter into HttpHandler and Sequence #315
Conversation
7b948b6
to
3c65e3f
Compare
The pull request is ready for review. |
Hmm, the code coverage is pretty bad, I'll need to add few more tests before this can be landed. Here are the least covered files:
|
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.
LGTM
@@ -30,7 +29,7 @@ describe('SwaggerRouter', () => { | |||
givenControllerClass(HelloController, spec); | |||
}); | |||
|
|||
it('handles simple "GET /hello" requests', () => { | |||
it('handles simple "GET /hello" requests', async () => { |
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.
remove async
59d0e37
to
2764605
Compare
Building on top of the decoupling work that has already landed, build the first full Sequence object to see where we are. Important: this is not the final implementation, just something that works end-to-end and can serve as a good base for further iterations. The few TODOs left in the code are intentional, they'll allow us to land this patch sooner.
I ended up with a new class
HttpHandler
which provides a method to handle incoming HTTP request. This method composes RoutingTable, Sequence and bits of custom code to invoke the correct controller method in the correct way. I expect these bits to be moved around as we iterate on the Sequence design and implementation.The existing
Application
class wrapsHttpHandler
and provides sugar API for registering controllers. TheServer
class is requiring a singleApplication
instance now and passes all requests to this single app.Connect to #308
See also strongloop-archive/loopback-next-sandbox#1
cc @bajtos @raymondfeng @ritch @superkhau @deepakrkris
Tasks:
Application.handleHttpRequest(req, res)
, move most of the request handling code from Server to Application