-
Notifications
You must be signed in to change notification settings - Fork 6
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-fe: useLocalStorageState 구현 #964
Conversation
1734510030.794339 |
1734510033.581279 |
1734593411.755679 |
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.
안녕하세요 러기~ 문제가 없어 보여 approve 드려요.
state가 변경될 때마다 브라우저에 쓰기가 일어나는 게 성능에 문제가 없는지 궁금해서 좀 찾아보느라 리뷰가 조금 늦어졌습니다.
스토리지 쓰기 자체는 매우 빨라서 고려할 수준은 아니고, 추후에 렌더링이 버벅인다거나 하면 throttle을 걸어서 대처하면 될 것 같네요!
고생하셨습니당.
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.
러기 안녕하세요. 구현해 주신 코드 감사히 확인했어요. 기능/테스트 측면에서 특별히 피드백 드릴 부분은 없고, 다만 Prop과 에러 처리에 대해 간단히 코멘트만 남겨드렸습니다. 살펴보시고, 반영이 필요하다고 생각되는 부분만 체크해주시면 감사하겠습니다.
try { | ||
const storedValue = window.localStorage.getItem(key); | ||
return storedValue !== null ? JSON.parse(storedValue) : initialValue; | ||
} catch (error) { | ||
return initialValue; | ||
} |
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에서 초기값을 로드할 때 에러가 발생하면 조용히 initialValue
를 리턴하도록 되어 있네요. 이대로도 문제는 없겠지만, process.env.NODE_ENV === 'development'
일 때 console.error
를 띄워주는 방식이 어떨지 제안드려 봅니다.
try { | ||
window.localStorage.setItem(key, JSON.stringify(value)); | ||
} catch (error) { | ||
console.error(`"${key}":`, error); |
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.
여기도 마찬가지로, 로컬 개발 환경에서만 콘솔 에러가 뜨도록 process.env.NODE_ENV === 'development'
조건을 붙여주면 어떨까요?
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.
에러처리에 대한 코드 피드백 반영하겠습니다
interface OptionProp { | ||
enableStorage: boolean; | ||
} |
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.
현재는 OptionProp
에 boolean
값을 가지는 enableStorage
필드만 정의되어 있어요. 혹시 다른 추가적인 옵션 prop을 염두에 두고 설계하신 것일까요?
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.
다른 Prop을 염두한 건 아니지만, 객체로 받은 이유는 enableStorage값이 boolean값이기 때문에 인자로 받게 될 경우 너무 flag스러웠기 때문입니다.
이 때문에 인자로만 받을 경우 가독성이 떨어졌습니다.
해당 Prop도 조금 더 개선하겠습니다!
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.
러기 피드백 반영해 주신 내용 모두 확인했어요. 특히 JSON 타입 데이터의 parsing, stringify 로직을 깔끔하게 분리해주신 부분이 정말 좋았습니다. Approve 드릴게요. 감사합니다 👍
const safeParseJSON = <T>(value: string | null, fallback: T): T => { | ||
try { | ||
return value !== null ? JSON.parse(value) : fallback; | ||
} catch (error) { | ||
if (process.env.NODE_ENV === 'development') { | ||
console.error('JSON 파싱 실패:', error); | ||
} | ||
return fallback; | ||
} | ||
}; | ||
|
||
const safeStringifyJSON = <T>(value: T): string | null => { | ||
try { | ||
return JSON.stringify(value); | ||
} catch (error) { | ||
if (process.env.NODE_ENV === 'development') { | ||
console.error('JSON 직렬화 실패:', error); | ||
} | ||
return null; | ||
} | ||
}; | ||
|
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.
parsing, stringify 로직을 깔끔하게 분리해주셨네요 👍
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Jeongwoo Park <wahoo9040@naver.com>
Original issue description
목적
#954
작업 세부사항
참고 사항
local-storage-state
closes #963
🛠️ useLocalStorageState 훅 구현
목적
useLocalStorageState 훅은 폼 요소 작성 중 사용자가 페이지를 이탈하거나 브라우저를 닫아도 데이터를 유지하여 사용자 경험을 개선하기 위해 설계된 훅입니다.
이 훅을 기반으로 React State와 LocalStorage를 연결하려 합니다.
주요 기능
✅ 테스트 코드 작성
테스트는 원시값에 대한 테스트와, 객체값에 대한 테스트로 나누었습니다.
테스트 설명
추가적으로 테스트해야될 케이스가 있다면 공유해주세요.
🔧 리팩터링