Feat/#14 JS SDK mvp1을 제작한다 #35
Conversation
- npm init - 배포를 위한 wepack bundler 추가 - 개발을 위한 타입스크립트 추가
- 빌드 디렉터리 - node modules - IDEA 설정파일
- 기존 console.log, console.error를 재정의 - app_id 를 초기화 할 수 있도록 설정 - 사용 예시는 문서화 예정
- 초기화 함수가 여러번 호출되어 여러번 초기화 되는것을 방지 - 제대로 console이 오버라이드 되지 않는 문제 해결
- Jest 도입
LuizyHub
added
📑 Docs
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| .Spotlight-V100 | ||
| .Trashes | ||
| ehthumbs.db | ||
| Thumbs.db No newline at end of file |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰입니다.
-
문법 및 형식: 패치는
.gitignore파일을 추가하고 있는 것으로 보입니다. 전반적으로 형식이 잘 되어 있습니다. -
버그 위험: 현재로서는 명백한 버그는 없습니다. 하지만,
Thumbs.db와 같은 특정 파일은 Windows에서만 생성되므로, 다른 운영 체제를 사용하는 경우에는 필요하지 않을 수 있습니다. 각 팀원의 OS에 맞춰 추가/제거할 필요가 있을 수 있습니다. -
개선 사항:
- 비슷한 파일 또는 디렉토리를 그룹화하여 가독성을 높일 수 있습니다. 예를 들어, OS 관련 항목들을 한 섹션으로 묶고 주석을 추가하면 좋습니다.
- 필요 없는 파일의 예시는 팀별로 별도의 검토가 필요할 수 있으니, 팀원들과 상의하여 일반적으로 필요한 항목들은 유지하고, 불필요한 것들은 제거하는 것도 고려해보세요.
-
새 줄: 파일 끝에 새 줄이 없으므로, 이 점도 수정하는 것이 좋습니다. 대다수의 코드 스타일 가이드에서는 파일 끝에 빈 줄을 권장합니다.
전반적으로 잘 작성된 패치이며, 몇 가지 개선 포인트를 반영하면 더 좋을 듯합니다.
sdk/js/README.md
Outdated
|
|
||
| - SDK를 사용하기 전에 반드시 `init` 메서드를 호출해야 합니다. | ||
| - `init` 메서드를 여러 번 호출해도 안전합니다. 중복 호출은 무시됩니다. | ||
| - 로그 전송 실패는 애플리케이션의 정상적인 실행을 방해하지 않습니다. No newline at end of file |
There was a problem hiding this comment.
코드 리뷰를 아래와 같이 진행하겠습니다.
-
문서화 및 설명:
- 기본적인 사용법과 특징이 잘 정리되어 있습니다.
- 예제 코드가 충분히 포함되어 있어 사용자 이해도를 높입니다.
-
초기화 방법:
LogBat.init메서드를 사용하기 전의 주의사항 언급은 좋습니다. 그러나,appId에 대한 명확한 설명(예: 반드시 유효한 값이어야 한다는 점)도 필요해 보입니다.
-
오류 처리:
- 로그 전송 실패 시 애플리케이션에 미치는 영향을 최소화한다고 했지만, 실패했을 때의 구체적인 처리 방법이나 대처 방안에 대한 추가 설명이 있으면 좋겠습니다.
-
코드 스타일:
- 코드 스타일이 일관되고 깔끔합니다. 다만, 긴 일정의 주석부 또는 오프라인 모드에서도 사용할 수 있는지에 대한 정보가 추가되면 사용자에게 더 도움이 될 것입니다.
-
다양한 환경 지원:
TypeScript 지원을 언급했으나, TypeScript 사용법에 대한 간단한 예시도 제공하면 타입 안전성을 중시하는 개발자에게 유용할 것입니다.
-
리소스 관리:
- SDK 초기화 후 리소스 해제에 대한 내용이 없는 것 같습니다. 불필요한 메모리 사용을 피하기 위해 종료 메서드를 제공하는 것도 좋은 개선점이 될 수 있습니다.
-
테스트 케이스:
- SDK의 기능들이 실제로 어떻게 작동하는지 검증할 수 있는 테스트 케이스 제공도 추천합니다.
총평: 전체적으로 문서 작성이 잘 되어 있으며, 기본적인 구조와 흐름이 명확합니다. 위의 제안 사항들을 고려하여 개선하면 사용자 경험을 더욱 향상시킬 수 있을 것입니다.
| testEnvironment: 'node', | ||
| testMatch: ['**/__tests__/**/*.ts?(x)', '**/?(*.)+(spec|test).ts?(x)'], | ||
| moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'json', 'node'], | ||
| }; No newline at end of file |
There was a problem hiding this comment.
코드 패치를 검토해보겠습니다.
-
이슈: 파일 끝에 개행 없음
- 대부분의 코드 스타일 가이드에서 파일의 끝에 개행을 권장합니다. 마지막 줄에
\ No newline at end of file메시지가 있다면 이 부분을 추가해야 합니다.
- 대부분의 코드 스타일 가이드에서 파일의 끝에 개행을 권장합니다. 마지막 줄에
-
전반적인 설정
ts-jest와node환경은 TypeScript와 Node.js 테스트에 적합하며, 기본 설정으로 문제가 없어 보입니다.testMatch배열에서 패턴을 잘 정의했습니다. 하지만 프로젝트 내 테스트 파일 경로가 표준 경로에 위치하도록 확인해야 합니다.
-
확장자 일관성
moduleFileExtensions에 포함된 모든 확장자가 사용되는지 확인하십시오. 필요 없는 확장자는 제거하여 파일 처리 속도를 높일 수 있습니다.
-
추가 설정 고려 사항
- 필요에 따라
collectCoverage나verbose같은 추가 설정을 고려할 수 있습니다. 코드 커버리지를 추적하고 테스트 결과를 명확히 하기 위해 유용할 수 있습니다.
- 필요에 따라
-
문서화
- 팀원이나 후속 개발자를 위해 이 설정 파일에 대한 간단한 설명 주석을 추가하는 것이 좋습니다.
이 외에 특별한 버그 리스크는 없어 보이며, 전체적으로 문제 없는 설정입니다.
| "webpack": "^5.93.0", | ||
| "webpack-cli": "^5.1.4" | ||
| } | ||
| } |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰입니다.
버그 위험
- 버전 호환성:
typescript,ts-loader,jest및ts-jest의 버전이 서로 호환되지 않을 가능성이 있습니다. 각 패키지의 호환되는 버전을 확인하는 것이 좋습니다.
개선 사항
- 스크립트 소개 추가: 프로젝트를 처음 사용하는 다른 개발자를 위해 스크립트에 대한 설명 주석을 추가하면 유용합니다.
- 저자 및 설명 필드 채우기:
author및description필드를 채워서 프로젝트에 대한 정보를 명확히 하는 것이 좋습니다. - 키워드 추가:
keywords필드에 적절한 키워드를 추가하면 검색성과 가독성을 높일 수 있습니다. - 의존성 관리:
devDependencies대신 필요한 경우dependencies로 분류할 의존성이 있는지 검토하세요. - 최신 버전 검토: 사용 중인 패키지의 최신 버전을 정기적으로 확인하고 업데이트하는 것이 바람직합니다.
이 외에도 지속적인 코드 품질 관리를 위해 테스트 커버리지를 체크하는 것도 추천합니다.
| await new Promise(resolve => setTimeout(resolve, 0)); // Allow async operations to complete | ||
| expect(consoleErrorSpy).toHaveBeenCalledWith('Regular error message'); | ||
| }); | ||
| }); No newline at end of file |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
-
Mocking 처리:
global.fetch를 모킹하는 과정은 괜찮습니다. 그러나 다양한 상황을 고려하여 응답을 다양화할 수 있습니다 (예: 실패 응답, 예기치 않은 데이터 구조 등). -
비동기 처리: 비동기 코드를 테스트할 때는
await사용이 적절하지만, 마지막 두 테스트에서는 포함된 로그가 비동기적으로 처리되므로afterAll이나 동일한 구조에서 실제LogBat메서드를 호출하는 것이 좋습니다. -
‘appId’ 노출:
LogBat['appId']접근 방식 대신, 전용 getter 함수를 사용하는 것이 더 안전하고 유지보수에 용이합니다. -
테스트 이름: 테스트 이름이 명확하지만, 각 테스트가 구체적으로 어떤 상황을 다루고 있는지 더욱 상세하게 기술하면 좋습니다(예: "정상 로그 메시지 보내기").
-
console.log와 console.error 사용: 메시지를 확인하는 부분에서 유의해야 할 점은, 별도의 로깅 라이브러리나 출력을 사용할 경우 해당 구현이 반드시 console에 의존하지 않아도 됩니다. 이런 점을 감안해, 콘솔 스파이 외에도 다른 로그 접근 방식을 사용할 수 있도록 개선할 수 있습니다.
-
네트워크 오류 핸들링: 현재의 테스트는 네트워크 오류를 잘 처리하고 있지만, 이와 함께 다양한 종류의 오류(예: 404, 500)도 고려하여 추가적인 테스트 케이스를 작성하는 것이 좋습니다.
-
No newline at end of file: 파일 끝에 새 줄이 없는 점은 일반적으로 권장되지 않습니다. 이는 일부 도구에서 문제를 일으킬 수 있으므로 수정하는 것을 권장합니다.
종합적으로, 기능은 거의 완벽하지만 더욱 다양한 시나리오를 고려하면 좋을 것 같습니다.
| } | ||
| } | ||
|
|
||
| export default LogBat; No newline at end of file |
There was a problem hiding this comment.
코드 리뷰 결과입니다.
버그 리스크:
-
appId값 검증 부족:init메서드가 호출될 때appId가 유효한지 검증하는 로직이 없어서 빈 문자열이나 잘못된 형식의 값을 설정할 위험이 있습니다. -
비동기 오류 처리:
log및error메서드 내에서 비동기 오류 발생 시, 원래 콘솔 메서드에 기록하려는 로그 메시지가 누락될 수 있습니다. (예:this.log(...args)가 실패하면this.originalConsole.log가 호출되지 않음) -
전역 상태 변경:
console.log와console.error를 직접 덮어쓰는 것은 다른 라이브러리와 충돌할 수 있는 잠재적인 버그로 이어질 수 있습니다.
개선 제안:
-
초기화 시
appId검증:init메서드에서config.appId가 빈 문자열이거나 유효하지 않은 경우 예외를 throw하도록 수정하세요. -
로그 함수 재구성:
log와error메서드는 동시에 콘솔 출력과 로그 전송을 하므로, 이를 Promise.all로 실행하여 성능을 개선할 수 있습니다.await Promise.all([this.sendLog(level, args), this.originalConsole.log(...args)]);
-
전역 콘솔 매서드를 복원할 수 있는 방법 제공: 원래 콘솔 메서드를 복원하는 기능을 추가하는 것이 좋습니다. 이럴 경우, 초기화 해제 메서드 등을 제공할 수 있습니다.
-
HTTP 요청 재시도 로직 고려: 네트워크 오류가 발생하는 경우에 대한 재시도 로직을 고려해 볼 수 있습니다.
-
타입 강화:
args의 타입을 any 대신, 구체적인 유형으로 정의하면 코드 가독성을 높이고, 잠재적인 오류를 줄일 수 있습니다. -
테스트 케이스 추가: 코드가 예상대로 작동하는지를 검증하기 위해 단위 테스트 및 통합 테스트를 추가하시길 권장합니다.
이러한 사항들을 반영하면 코드의 안정성과 유지보수성이 향상될 것입니다.
| "esModuleInterop": true, | ||
| }, | ||
| "include": ["src/**/*"] | ||
| } No newline at end of file |
There was a problem hiding this comment.
코드 패치에 대한 간단한 리뷰를 제공합니다.
-
문법 오류:
esModuleInterop항목 뒤의 쉼표(,)는 JSON 형식에서는 필요하지 않습니다. 이를 제거해야 합니다. -
출력 디렉토리:
outDir을 "./dist/"로 설정했는데, 이런 경로가 존재하는지 확인하고, 빌드 프로세스에서 해당 폴더를 적절히 정리해야 할 수도 있습니다. -
타겟 버전:
target을 "es5"로 설정했지만, 만약 최신 기능을 사용해야 한다면 ES6 이상으로 업데이트하는 것도 고려해볼 수 있습니다. -
allowJs속성: JavaScript 파일도 포함할 수 있게 허용하고 있는데, TypeScript 프로젝트에서 불필요한 JavaScript가 포함될 위험이 있으므로 주의가 필요합니다. -
include패턴:"src/**/*"로 설정하여 src 디렉토리 내 모든 파일을 포함시키는데, 필요한 경우 특정 파일만 포함하도록 필터링할 수 있습니다.
전체적으로 잘 구성된 설정이지만 위의 몇 가지 사항을 점검하고 수정하면 좋겠습니다.
| minimize: isProduction | ||
| } | ||
| }; | ||
| }; No newline at end of file |
There was a problem hiding this comment.
코드 리뷰를 진행하겠습니다.
잠재적인 버그 리스크
-
globalObject설정:globalObject: 'this'로 설정된 경우, 브라우저와 Node.js 환경에서 다르게 동작할 수 있습니다. 특정 상황에서는globalThis를 사용하는 것이 더 안전합니다. -
ts-loader의존성:ts-loader가 제대로 설치되지 않았다면 빌드 오류가 발생할 수 있습니다. 이를 위한 확인 절차가 필요합니다.
개선 제안
-
출력 파일 이름: 현재
filename이 환경변수에 따라 결정되는데, 이를 좀 더 명확하게 관리할 수 있는 변수를 사용하는 것이 좋습니다. 예를 들어,${isProduction ? 'production' : 'development'}.sdk.js와 같이 정리할 수 있습니다. -
Webpack의 캐시: 개발 환경에서의 빌드를 가속화하기 위해 캐시 사용을 고려할 수 있습니다 (
cache: { type: 'filesystem' }). -
환경 변수 관리: 나중에 다양한 환경 구성을 고려한다면, 외부
.env파일이나 별도의 설정 파일을 도입하여 관리하는 방법도 검토해 볼 만합니다. -
주석 추가: 각 섹션의 코드에 대한 주석을 추가하여 코드의 가독성을 높일 수 있습니다.
-
새 줄 추가: 파일 끝에 새 줄을 추가함으로써 툴 호환성 문제를 피할 수 있습니다.
모두 반영하면 더욱 안정적이고 효율적인 코드를 작성할 수 있을 것입니다.
|
|
||
| - SDK를 사용하기 전에 반드시 `init` 메서드를 호출해야 합니다. | ||
| - `init` 메서드를 여러 번 호출해도 안전합니다. 중복 호출은 무시됩니다. | ||
| - 로그 전송 실패는 애플리케이션의 정상적인 실행을 방해하지 않습니다. |
There was a problem hiding this comment.
코드 패치에 대한 간단한 코드 리뷰입니다.
잠재적 버그 위험
- 초기화 누락:
LogBat.init이 호출되지 않으면 로그가 전송되지 않는데, 사용자가 이를 잊을 수 있습니다. 초기화 여부를 확인하는 로직을 추가하면 좋습니다. - 네트워크 오류 처리: 로그 전송 실패에 대한 처리 로직이 부족해 보입니다. 예를 들어, 실패 시 재시도 메커니즘이나 사용자에게 알림을 제공하는 방법이 필요합니다.
개선 제안
- 문서화 강화: 각 메서드의 기능과 인자에 대한 설명을 자세히 추가하면 사용자가 SDK를 보다 쉽게 이해하고 사용할 수 있습니다.
- 비동기 함수 사용: 로그 전송 기능을 비동기적으로 처리하여 성능을 더욱 향상시킬 수 있습니다.
fetch또는XMLHttpRequest를 사용하는 것이 좋습니다. - 구성 파라미터 검증:
LogBat.init의appId와 같은 필수 파라미터에 대한 유효성 검사를 추가하는 것이 좋습니다. - 예외처리: 로그 캡처 과정에서 발생할 수 있는 예외를 처리하여 앱 크래시를 방지해야 합니다.
이러한 점들을 고려하여 개선한다면 SDK의 안정성과 사용성을 높일 수 있을 것입니다.
* config: 프로젝트 세팅 - npm init - 배포를 위한 wepack bundler 추가 - 개발을 위한 타입스크립트 추가 * config: gitignore 파일 추가 - 빌드 디렉터리 - node modules - IDEA 설정파일 * feat: LogBat sdk 추가 - 기존 console.log, console.error를 재정의 - app_id 를 초기화 할 수 있도록 설정 - 사용 예시는 문서화 예정 * fix: cosole 바인딩 문제 해결 - 초기화 함수가 여러번 호출되어 여러번 초기화 되는것을 방지 - 제대로 console이 오버라이드 되지 않는 문제 해결 * cofing: 테스트 라이브러리 추가 - Jest 도입 * test: SDK 테스트 추가 * docs: README.md 추가 * fix: sdk 초기회 예시 변경
🚀 개발 사항
이슈 번호
특이 사항 🫶
sdk/js/src/index.ts에 존재합니다sdk/js/README.md에 있습니다