-
Notifications
You must be signed in to change notification settings - Fork 22
[박민규] sprint 11 #43
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
[박민규] sprint 11 #43
Conversation
This reverts commit 95374ec.
basilry
left a comment
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.
매우 완성도 높게 잘 하셨습니다!
프론트도 그랬지만 정말 퀄리티가 높으시네요.
크게 짚을 것은 없었고, 짜잘한 것들 위주였습니다. 고생하셨습니다.
| @@ -1,59 +1,48 @@ | |||
| # 게시글 API | |||
| @accessToken = eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VySWQiOiJjbWJya25tZGwwMDAwaHAzMmhvcjYxaTR2IiwiaWF0IjoxNzUwMTQyMTQ2LCJleHAiOjE3NTAxNDMwNDZ9.8yLrA7X_7RZsl25lvViSBOIlYUReZKWII-FSPLv3g5g | |||
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.
음.. 다른 분께도 말씀드렸지만, 혹시나 다른 프로젝트에서도 이렇게 토큰을 레포에 올리심 안됩니다ㅠㅠ
| search?: string; | ||
| sort?: string; | ||
| } | ||
| >, |
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.
어... 제네릭을 이런식으로 안하셔도 되지 않았을까 합니다.
어차피 Request타입만 적으셔도 req.query.~~ 로 접근해서 처리가 가능하지 않나? 싶네요.
|
|
||
| // 좋아요 취소 | ||
| const unlikeArticle = async (req, res, next) => { | ||
| const unlikeArticle = 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.
해당파일을 쭉 보니, 어느 api 함수는 리턴메시지가 있고, 어느 함수는 없네요.
통일성 있게, 그리고 협업할 때 프론트 개발자가 편할 수 있도록, 리턴메시지는 보내줍시다.
| req: Request<{}, {}, UserSignInDto>, | ||
| res: Response, | ||
| next: NextFunction | ||
| ) => { |
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.
이거 const signIn: RequestHandler = async (req, res, next) => {...로 처리 가능할 것 같네요.
req로 들어오는 데이터의 dto를 설정하고 싶다고 하신다면.. 음..
express에서는 크게 dto 가드 같은게 의미가 없다고 말씀드릴 수 있겠네요.
요거는 nest.js 같은 걸 쓰셔야 좀 유의미하게 사용 가능합니다. 한 번 찾아보세요!
|
|
||
| await userService.updateUser(user.id, { refreshToken }); | ||
|
|
||
| res.status(201).json({ user, accessToken, refreshToken }); |
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.
리턴메시지가 있었으면 좋겠네요. 같은 내용은 더 답글 달지 않겠습니다.
|
|
||
| // 게시글 댓글 목록 조회 | ||
| const getCommentsByArticleId = async (req, res, next) => { | ||
| const getCommentsByArticleId: RequestHandler = async (req, res, 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.
오잉 여기는 위처럼 안쓰셨군요..
|
|
||
| // tags 데이터 처리: 문자열로 전송된 경우 배열로 변환 | ||
| let processedTags = tags; | ||
| let processedTags: string[] = Array.isArray(tags) ? tags : []; |
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.
이거 보아하니 tags 데이터 처리랑 밑의 여러 이미지 파일 처리 로직이 다른 곳에서도 중복되는 것 같군요.
이런 거는 lib/util.ts나 lib/root.ts에 넣어서 공통화 합시다.
| /** | ||
| * 댓글 상세 조회 | ||
| */ | ||
| const findById = async (id: CommentParamsDto["id"]) => { |
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.
이런 형식의 타입할당이 반복되는데, 그냥 string으로 안쓰신 이유가 있을까요?
물론 통일성 있게 그냥 전체 프로젝트가 이런식으로 가겠다고 한다면,
그건 그 자체로 컨벤션이므로 큰 문제는 되지 않습니다.
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.
저도 고민한 부분인데 강사님께 여쭤보니 멘토님과 동일하게 컨벤션의 문제라고 하셔서 이번 프로젝트에선 좀 더 가독성을 올리는 의미에서 "인덱스 접근 타입"을 사용하였습니다
| try { | ||
| // 사용자의 refreshToken을 null로 업데이트 | ||
| await userRepository.update(userId, { refreshToken: null }); | ||
| await userRepository.update(userId, { refreshToken: undefined }); |
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.
어.. null도 문제지만, undefined도 문제입니다. 차라리 빈문자열로 저장하시는게 낫지 않을까요?
📝 위클리 미션 PR
✅ 공통 요구사항
any타입 사용을 최소화하였습니다.🔧 백엔드 (Express.js)
tsconfig.json을 생성하고 필요한 옵션(outDir,rootDir,strict, 등)을 설정하였습니다.package.json에 빌드 및 개발 서버 실행 스크립트를 설정하였습니다."dev": "nodemon src/app.ts"ts-node와nodemon을 활용하여 개발 환경을 구성하였습니다..ts파일 수정 시 서버가 자동으로 재시작되도록 설정 (nodemon + ts-node조합)declare module을 사용해 타입을 오버라이드 또는 확장하였습니다.