-
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
refactor(rest): use Express req/res types #1326
Conversation
@@ -14,14 +14,14 @@ import {AuthenticationBindings} from '../keys'; | |||
* Passport monkey-patches Node.js' IncomingMessage prototype | |||
* and adds extra methods like "login" and "isAuthenticated" | |||
*/ | |||
export type PassportRequest = ParsedRequest & Express.Request; | |||
export type PassportRequest = Request & Express.Request; |
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.
Do we still have to keep & Express.Request
?
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.
Good catch, I'll refactor and clean up the authentication module as part of this PR.
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.
Actually, Express.Request
is not the Request object from express js! Instead, it's a passport interface describing additional API added by passport middleware to the request object.
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.
Please fix CI failures.
Please rebase to latest master to pick up a fix that passes the CI. |
6b31cc5
to
7099c99
Compare
Please fix the lint error before merging:
|
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.
Should https://github.com/strongloop/loopback-next/blob/c271b25245889834581aa5e9803fd43ef6029072/packages/rest/src/rest.server.ts#L345-L348 be updated to be Request
/Response
instead of ServerRequest
/ServerResponse
?
@@ -4,7 +4,7 @@ | |||
// License text available at https://opensource.org/licenses/MIT | |||
|
|||
import {Strategy, AuthenticateOptions} from 'passport'; | |||
import {PassportRequest} from '../../..'; | |||
import {Request} from 'express'; |
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.
Shouldn't this be imported from @loopback/rest
?
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.
Here is my reasoning: MockStrategy
is extending from Passport Strategy
, which uses Request
from express
.
If we ever change @loopback/rest
to export Request
as a different type, then MockStrategy still needs to keep using Request
from express
, because that's what the Strategy
interface requires.
import {RestBindings} from './keys'; | ||
import {RequestContext} from '.'; | ||
import {RequestContext} from './request-context'; | ||
import * as express from 'express'; | ||
|
||
export type HttpRequestListener = ( | ||
req: ServerRequest, |
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.
Should this be Request
not ServerRequest
?
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.
No. HttpRequestListener
is passed to http.createServer
, it must work with core ServerRequest
/ServerResponse
(no Express additions).
@@ -8,6 +8,8 @@ | |||
* https://github.com/hapijs/shot | |||
*/ | |||
|
|||
// tslint:disable:no-any | |||
|
|||
import {ServerRequest, ServerResponse} from 'http'; |
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.
Shouldn't this be Request
/ Response
from express
now?
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.
There are two ways how to use these Shot helpers:
-
To test LB4 code like a Sequence action. To do that, we need to mock Express
Request
/Response
objects. -
To test LB4 RestServer's request handler in isolation, without calling
http.createServer
. In this case, we need to mock core HTTPServerRequest
/ServerResponse
objects.
That's why I am importing both Express and core HTTP types.
@virkt25 regarding _redirectToSwaggerUI - good catch! I'll also leverage |
Redesign the REST layer to internally assume than Express has already processed and enhanced the request and response objects. The public API for creating a listening HTTP server remains unchanged for now, i.e. our RestServer can be still used directly with the core http/https Server classes.
Now that the REST layer uses Express request/response types internally, we can simplify StrategyAdapter by removing ShimRequest. While making these changes, other places are cleaned up as well.
c271b25
to
8925db8
Compare
Landed. I'll address any additional comments in a follow-up pull request if needed. |
Redesign the REST layer to internally assume than Express has already processed and enhanced the request and response objects.
The public API for creating a listening HTTP server remains unchanged for now, i.e. our RestServer can be still used directly with the core http/https Server classes.
This pull request is a follow-up for #1082 and a part of #1253, it also fixes #191.
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated