-
Notifications
You must be signed in to change notification settings - Fork 0
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
[FEAT] LogScreen, Log 로컬스토리지 저장 #10
Conversation
src/types/LogPayloadParams.ts
Outdated
name: string; | ||
export interface LogPayloadParams { | ||
userId: number; | ||
screenName: 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.
엇 네이밍이 일치하지 않는 것 같네용 로그 모델과 네이밍이 통일되면 좋을 것 같아요!
{
"userId": "${random-value}", // 유저를 특정할 수 없는 값, 로깅 시스템 내에서만 구별할 수 있도록
"timestamp": "2023-12-04T10:30:00Z",
"event": {
"platform": "Web",
"serviceName": "drawer",
"name": "SearchButtonClicked",
"message": "Clicked search button",
"tags": ["search", "button", "click"],
}
}
screenName -> serviceName
eventName -> name
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.
screenName은 /drawer/1 eventName은 버튼 클릭 이런걸 의도 했던건데 serviceName자체가 너무 포괄적인것 같은데.. 어떻게 하면 좋을까요?
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.
serviceName은 drawer, search, home 3가지구 name은 버튼 클릭 같은 내용이에요! 요건 다른 팀들이랑 공통으로 가져가는 거라 통일하면 좋을 것 같슴다..!!! 말씀해주신 drawer 하위 페이지들은 path에 넣으면 될 것 같아용!
yarn.lock
Outdated
@@ -2635,6 +2635,11 @@ uri-js@^4.2.2: | |||
dependencies: | |||
punycode "^2.1.0" | |||
|
|||
uuid@^9.0.1: |
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.
혹시 어떤 package 추가하신 걸까요? 삭제가 안 된건가 해서요!
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.
앗 이거 삭제해 둘게용!
src/Logger.ts
Outdated
}, | ||
events: events, |
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.
혹쉬 event와 별개로 events는 어떤 역할인가용?
src/Logger.ts
Outdated
|
||
const click = ({ name }: LogPayloadParams) => { | ||
console.log(`Logging click information for button: ${name}`); | ||
if (window.localStorage.getItem('yls-web') == 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.
사실 localStorage 저장까지 같이 작업하실 줄 몰랐어서..!! localStorage 저장 로직은 따로 함수로 분리하는 게 좋을 것 같은데 어떻게 생각하시나용?
앗 그리구 useEffect로 인해서 localStorage에 로그가 중복 생성되는 것 같아요!
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.
남겨주신 'LogScreen에서도 userId가 필요할지'는 yes인 것 같습니다! 아무래도 LogClick보다 LogScreen이 먼저 일어날 수 밖에 없는데 사용처에서 처음에 userId를 넣어줘야 이후에 저희가 생성한 해시 값으로 사용할 수 있을 것 같네용
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️⃣ 어떤 작업을 했나요? (Summary)
기존 코드에 영향을 미치는 변경사항
2️⃣ 알아두시면 좋아요!
[demo]
react router App.tsx에 한번 씌워서 Home.tsx에 Hanna님 코드도 넣어놨어요! 그 useLocation()에서 path받아오는걸 하고 싶었는데 그러려면 무조건 Router환경에서 해야해서 이렇게 바꿔뒀습니당!
[LogPayloadParams]
LogPayloadParams 에서 name을 eventName과 screenName으로 나눴습니당
createRandomId()의 경우 나중에 로그 자체 식별id만들때를 대비하여 헷갈리지 않게 하기 위해 이름을 createUserId()로 바꿨습니다!
[로컬스토리지 저장]
우선 Key값은 yls-web으로 하고 저장했는데요.지금 Logger이랑 useYLSLogger()가 뭔가 따로 노는것 같거든요.. 근데 로컬스토리지에 넣으려면 백엔드에 보내야하는 데이터 포멧 그대로 스토리지에 들어가야하고, 그러려면 Logger에 있는 포멧을 따라야하는데 이게 뭔가 좀 뒤죽박죽이라 어렵네여 ㅜ
[userId관련]
Logscreen에서도 굳이 userId가 필요할지 -> contextAPI쓰는거 까지한다면 굳이 안필요할거 같아용
3️⃣ 추후 작업
4️⃣ 체크리스트 (Checklist)
main
브랜치의 최신 코드를pull
받았나요?