Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(@yourssu/logging-system): fix 10th lost-log #68

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat(@yourssu/logging-system): fix 10th lost-log #68

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
524ac65
fix: 10번째 로그 쌓일 시 즉시 전송
owl1753 Sep 25, 2024
e1225c4
fix: 전송에 실패한 로그 스토리지에 쌓기
owl1753 Sep 25, 2024
1e1a3ae
fix: 보내지지 않은 로그는 다시 로컬 스토리지에 쌓이도록 수정
owl1753 Oct 30, 2024
5d44077
fix: 로그가 10개 쌓이면 전송
owl1753 Nov 6, 2024
7f21067
Merge branch 'main' into fix/lost-log
owl1753 Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 18 additions & 22 deletions packages/logging-system/src/SetLocalStorage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,32 @@
import { LogRequestList, LogResponse, LogType } from './types/LogType';

export const SetLocalStorageClear = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const SetLocalStorageClear = () => {
export const ClearLocalStorage = () => {

이것도 네이밍이 미묘하네요.

const list: any[] = [];
const list: LogType[] = [];
localStorage.setItem('yls-web', JSON.stringify(list));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
localStorage.setItem('yls-web', JSON.stringify(list));
localStorage.setItem('yls-web', "[]");

이거 그냥 이렇게 해도 되지 않나요?

};

export const SetLocalStorage = async (
Copy link
Member

Choose a reason for hiding this comment

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

PR의 내용과는 관계없습니다만, 이거 왜 SetLocalStorage인가요? 실제 하는 일은 로그를 전송하는 건데, 관심사 분리를 확실히 하고 로컬스토리지 저장과 로그 전송을 순차 실행하는 새로운 함수를 만드셔야 할 것 같습니다. 제가 YLS 코드에 대한 배경 지식이 없었는데, 지금 리뷰하면서 살펴보니 고쳐야 할 부분이 많아 보이네요.

예시) SetLocalStorage 함수를 storeLog, sendStoredLogs 함수로 분리 후 상위 호출 함수에서 두 함수를 순서대로 호출하도록 리팩토링

일단 이 PR과 관계없으므로 resolve 되지 않아도 Approve는 가능합니다.

logger: LogType,
Copy link
Member

Choose a reason for hiding this comment

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

이 PR만의 문제가 아닌 것 같긴 한데, 얘 이름이 logger인 이유를 모르겠네요. log 데이터 그 자체이므로 이름 수정이 필요합니다.

putLog: (data: LogRequestList) => Promise<LogResponse>
) => {
if (window.localStorage.getItem('yls-web') == undefined) {
const list: any[] = [];
list.push(logger);
localStorage.setItem('yls-web', JSON.stringify(list));
} else {
const remainList: any[] = JSON.parse(localStorage.getItem('yls-web') as string) || [];
if (remainList.length < 10) {
const updateList = [...remainList, logger];
localStorage.setItem('yls-web', JSON.stringify(updateList));
} else {
const req: LogRequestList = {
logRequestList: remainList,
};
const list: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) || [];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const list: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) || [];
const list: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) ?? [];

|| 말고 nullish coalescing 연산자(??) 활용하시면 의도에 맞으실 것 같습니다.

list.push(logger);
localStorage.setItem('yls-web', JSON.stringify(list));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
localStorage.setItem('yls-web', JSON.stringify(list));
localStorage.setItem('yls-web.logs', JSON.stringify(list));

yls-web.logs 처럼 데이터의 정확한 명칭을 지정해서 의도를 명확히 하고, 앞으로 다른 데이터가 추가되어도 네이밍 문제가 생기지 않도록 하면 좋겠습니다.


if (list.length >= 10) {
const req: LogRequestList = {
logRequestList: list,
};

SetLocalStorageClear();

try {
const res = await putLog(req);
if (res.success) {
SetLocalStorageClear();
}
} catch (e) {
console.error('Failed to post log');
}
try {
await putLog(req);
} catch (e) {
console.error('Failed to post log');
Copy link
Member

Choose a reason for hiding this comment

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

이 오류는 프로덕션에는 안나면 좋겠네요. 어짜피 Network 탭에서 요청 상태를 확인 가능하니까요.

Copy link
Member

Choose a reason for hiding this comment

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

혹시나 에러 로깅을 하고 싶으시다면 e도 포함해서 로깅해주시면 좋겠습니다.

Suggested change
console.error('Failed to post log');
console.error('Failed to post log', e);

const remainList: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) || [];
Copy link
Member

Choose a reason for hiding this comment

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

remainList라는게 아직 못 보낸 로그처럼 느껴져서 logsAfterRequest 같은 느낌으로 수정했으면 좋겠습니다.

remainList.unshift(...list);
localStorage.setItem('yls-web', JSON.stringify(remainList));
Comment on lines +28 to +29
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
remainList.unshift(...list);
localStorage.setItem('yls-web', JSON.stringify(remainList));
list.push(...remainList);
localStorage.setItem('yls-web', JSON.stringify(list));

이편이 깔끔하지 않을까요?

}
}
};