feat: Migrate Session.controller.js to typescript #3958
feat: Migrate Session.controller.js to typescript #3958Piyushrathoree wants to merge 4 commits intoprocessing:developfrom
Conversation
There was a problem hiding this comment.
This file does not need to be committed. Please exclude it from the commit.
| next(innerErr); | ||
| return; | ||
| } | ||
| res.json(userResponse(req.user!)); |
There was a problem hiding this comment.
A safer approach for this is res.json(userResponse(user));
| {}, | ||
| CreateSessionResponseBody, | ||
| CreateSessionRequestBody | ||
| > = (req: Request, res: Response, next: NextFunction) => { |
There was a problem hiding this comment.
Since RequestHandler<> is already used for typing, explicit Request, Response, and NextFunction annotations can be removed for cleaner code.
Piyushrathoree
force-pushed
the
ts/migrate-session-controller
branch
from
d76fec9 to
1c59e8d
Compare
- Add RequestHandler generics and JSDoc to session controller - Add server/types/session.ts with request/response types - Update server/types barrel to export session types
Piyushrathoree
force-pushed
the
ts/migrate-session-controller
branch
from
1c59e8d to
230bc07
Compare
|
@yugalkaushik made changes |
| @@ -0,0 +1,15 @@ | |||
| import { PublicUser, GenericResponseBody } from '.'; | |||
There was a problem hiding this comment.
this creates a circular dependency, please update to import from the correct source files
| | PublicUser | ||
| | { message: string }; | ||
|
|
||
| export type DestroySessionResponseBody = { success: boolean }; |
There was a problem hiding this comment.
there's already GenericResponseBody, I would either Pick from that, or just omit this change and use GenericResponseBody instead
| next(innerErr); | ||
| return; | ||
| } | ||
| res.json(userResponse(user)); |
There was a problem hiding this comment.
can you change this back to userResponse(req.user)? There are no tests for the controller yet, so it would be safer not to change this
clairep94
left a comment
clairep94
left a comment
There was a problem hiding this comment.
sorry misclicked when I hit approve, if you could amend per the feedback that would be great!
clairep94
added
the
Contributor Follow-up Required
3 participants
Fixes #3957
Changes:
server/controllers/session.controller.jsto exactly matching TS file.CreateSessionRequestBody,CreateSessionResponseBody,GetSessionResponseBody, andDestroySessionResponseBodytypes inserver/types/session.ts.@types/express-sessionas a devDependency to allow the global ExpressRequestobject to understandreq.session.destroy().I have verified that this pull request:
npm run lint)npm run test)npm run typecheck)developbranch.Fixes #123