Skip to content

#596 TS 마이그레이션 auth, auth.mobile, auth.replace, chats #601

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

Open
wants to merge 25 commits into
base: dev
Choose a base branch
from

Conversation

Halo-sparcs
Copy link

Summary

It closes #596

Extra info

  • auth.js
  • auth.mobile.js
  • auth.replace.js
  • chats.js
    를 ts로 마이그레이션 합니다.

@Halo-sparcs Halo-sparcs changed the title CREATE: 596 branch 596 ts migration auth, auth.mobile, auth.replace, chats May 20, 2025
@Halo-sparcs Halo-sparcs requested a review from kmc7468 May 20, 2025 12:53
@Halo-sparcs Halo-sparcs self-assigned this May 20, 2025
@Halo-sparcs Halo-sparcs marked this pull request as ready for review May 20, 2025 12:53
Copy link
Member

@kmc7468 kmc7468 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다! socket.js 파일도 같이 마이그레이션 부탁드려요

@kmc7468 kmc7468 changed the title 596 ts migration auth, auth.mobile, auth.replace, chats #596 TS 마이그레이션 auth, auth.mobile, auth.replace, chats Jul 1, 2025
@kmc7468
Copy link
Member

kmc7468 commented Jul 9, 2025

@Halo-sparcs JSDoc에 타입이 명시되어 있는 경우가 많은데, (예를 들면 services/chats.ts 파일에 있는 isUserInRoom 함수) TypeScript로 마이그레이션된 경우에는 이미 타입 정보가 함수 자체에 포함이 다 되어 있으니까, JSDoc에서 타입 정보만 삭제하면 어떨까요? 주석도 더 깔끔해질 것 같고, 혹시라도 생길 수 있는 JSDoc과 실제 타입 간의 불일치 문제도 차단할 수 있는 장점이 있는 것 같습니다.

스크린샷 2025-07-09 오전 11 16 31

(예를 들어 위 같은 경우 JSDoc에 있는 string, Promise을 없애자는 의견입니다!)

@kmc7468 kmc7468 requested a review from Copilot July 9, 2025 02:19
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR migrates the auth, auth.mobile, auth.replace, and chats service modules from JavaScript to TypeScript, adding type annotations, refining imports, and introducing runtime type guards.

  • Converted CommonJS require calls to ES module import syntax and added explicit types.
  • Introduced isChatType guard in chats.ts and strengthened socket event definitions in modules/socket.ts.
  • Updated authentication flows with typed request handlers and adjusted login/logout signatures.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/services/chats.ts Converted to TS, added RequestHandler types, isChatType guard and stricter .lean<>() calls
src/services/auth.ts Added interfaces for raw and transformed user data, typed handlers, updated login signature
src/services/auth.replace.ts Typed handlers, refined imports, non-null assertions on session data
src/services/auth.mobile.ts New TS handlers for token-based login/refresh and device registration
src/modules/socket.ts Typed Socket.IO server and events, defined ChatArrayObject, updated JSDoc
src/modules/auths/login.ts Made sid optional in session, adjusted login parameter order
Comments suppressed due to low confidence (1)

src/modules/socket.ts:43

  • The JSDoc for transformChatsForRoom still refers to an untyped Object[]; update it to reference ChatArrayObject[] or current TS types for accuracy.
 * @param {Object[]} chats - Chats Document에 lean과 populate(chatPopulateOption)을 차례로 적용한 Chat Object의 배열입니다.

@@ -137,11 +140,30 @@ const loadAfterChatHandler = async (req, res) => {
}
};

const sendChatHandler = async (req, res) => {
function isChatType(x: unknown): x is ChatType {
Copy link
Preview

Copilot AI Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type guard doesn’t include the "departure" chat type, which is valid in ChatType; add it to chatTypeValues so valid messages aren’t rejected.

Copilot uses AI. Check for mistakes.

Copy link
Member

@kmc7468 kmc7468 Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zod에서 enum으로 바꿔주면 이제 이 함수는 필요 없어질 것 같네영

@kmc7468
Copy link
Member

kmc7468 commented Jul 9, 2025

오.. 코파일럿 리뷰 꽤 괜찮네요! 이상하다 싶은 코멘트는 그냥 거르셔도 될 것 같습니다.

Copy link
Member

@kmc7468 kmc7468 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코멘트에 남겨 드린 것처럼, 라우터 쪽은 TS 마이그레이션이 안돼서 같이 작업이 필요할 것 같습니다!!

@@ -1,11 +1,11 @@
const { zodToJsonSchema } = require("zod-to-json-schema");
const logger = require("../../modules/logger").default;

const zodToSchemaObject = (zodObejct) => {
const zodToSchemaObject = (zodObject) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😁

const info = {
id: id,
sid: id + "-sid",
name: id + "-name",
nickname: generateNickname(id),
profileImageUrl: generateProfileImageUrl(),
facebook: id + "-facebook",
twitter: id + "-twitter",
kaist: "20220411",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

민후님 학번으로 업데이트 하셔서 흔적(?)을 남기시는건 어떨까요 (농담입니다)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS 마이그레이션 auth, auth.mobile, auth.replace, chats
2 participants