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

fix(@yourssu/logging-system): unload event 시 서버로 log 전송 #66

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
+25 −2
Open

fix(@yourssu/logging-system): unload event 시 서버로 log 전송 #66

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
77f9f2c
feat: visibilitychange event로 사용자 이탈 감지
owl1753 Oct 16, 2024
50af6fa
fix: useEffect에 의존성 배열 추가
owl1753 Oct 16, 2024
ad2900e
refactor: async/await 제거
owl1753 Oct 16, 2024
3441ca4
refactor: cleanup 함수 추가
owl1753 Oct 16, 2024
e40b3f4
remove: tcp keepalive 제거
owl1753 Nov 6, 2024
a309429
Merge remote-tracking branch 'origin/main' into fix/send-exit-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
1 change: 1 addition & 0 deletions packages/logging-system/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"react-router-dom": "^6.21.3"
},
"devDependencies": {
"@types/node": "^22.7.5",
"@types/react-router-dom": "^5.3.3",
"@yourssu/crypto": "workspace:*",
"axios": "^1.6.4",
Expand Down
26 changes: 25 additions & 1 deletion packages/logging-system/src/contexts/YLSProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import { createContext } from 'react';
import { createContext, useEffect } from 'react';

import { createAxiosInstance } from '@/apis/createAxiosInstance';
import { LogRequestList, LogResponse } from '@/types/LogType';
import { LogType } from '@/types/LogType';
import http from 'http';
import https from 'https';
import { SetLocalStorageClear } from '@/SetLocalStorage';

interface YLSProviderProps {
children: React.ReactNode;
Expand Down Expand Up @@ -32,6 +36,26 @@ export const YLSProvider = ({ children, baseURL }: YLSProviderProps) => {
}
};

useEffect(() => {
const handleVisibilityChange = async () => {
if (document.visibilityState === 'visible') return;

const logList: LogType[] = JSON.parse(localStorage.getItem('yls-web') as string) || [];
const data: LogRequestList = {
logRequestList: logList,
};

SetLocalStorageClear();

await axiosInstance.put('/log/list', data, {
httpAgent: new http.Agent({ keepAlive: true }),
owl1753 marked this conversation as resolved.
Show resolved Hide resolved
httpsAgent: new https.Agent({ keepAlive: true }),
owl1753 marked this conversation as resolved.
Show resolved Hide resolved
});
};

document.addEventListener('visibilitychange', handleVisibilityChange, { once: true });
});
Copy link

Choose a reason for hiding this comment

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

useEffect 내부에 적용해서 cleanup 있을 때 함께 정리할 수 있도록 하면 더 좋을 것 같아요.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

refactor: cleanup 함수 추가 cleanup 함수를 추가하라는 말씀이신가요?

Copy link
Member

Choose a reason for hiding this comment

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

refactor: cleanup 함수 추가
현재 코드에서 이 변경 사항 차이점이 있는 건가요?

Copy link

Choose a reason for hiding this comment

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

@Hanna922

refactor: cleanup 함수 추가 현재 코드에서 이 변경 사항 차이점이 있는 건가요?

YLSProvider보다 상위 Provider나 App 컴포넌트 내에서 리렌더링이 일어나면, 이벤트 리스너가 다시 등록 되면서, 메모리 누수의 가능성이 있을 것 같아요.

Copy link
Member

Choose a reason for hiding this comment

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

once: true 속성이 이벤트 리스너를 자동으로 제거하는 것으로 알고 있는데 제거 전에 리렌더링이 발생할 수 있다는 말씀이실까요??
remove 기능만 하는 거라면 변경 전과 동일한 것으로 알고 있어서 여쭤봅니당

Copy link

Choose a reason for hiding this comment

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

Provider 컴포넌트는 여전히 이벤트 리스너 호출 후 once: true로 지워지는 것보다 먼저 리렌더링 될 수 있어서 클린업을 해야 하지 않을까 싶어요


Copy link

Choose a reason for hiding this comment

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

Provider가 최상위에서 정의될텐데, 의존성 배열이 없으면 기대하지 않은 렌더링 상황에서 매번 호출될 것 같아요. (이벤트 리스너가 계속 등록되고 삭제될 듯합니다)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fix: useEffect에 의존성 배열 추가에서 수정했습니다.

return (
<YLSContext.Provider
value={{
Expand Down
Loading
Loading