feat(utils): parseJson 함수 추가 완료 #115
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #115 +/- ##
==========================================
+ Coverage 87.13% 87.31% +0.18%
==========================================
Files 68 69 +1
Lines 614 623 +9
Branches 140 142 +2
==========================================
+ Hits 535 544 +9
Misses 68 68
Partials 11 11
|
|
@Sangminnn PR 감사합니다! 내일중으로 검토하도록 하겠습니다! |
|
@ssi02014 감사합니다! 천천히 확인해주셔도 괜찮으니 편하게 봐주시면 감사드리겠습니다! :) |
| export const parseJSON = <T>(value: T) => { | ||
| if (typeof value !== 'string') { | ||
| return value; | ||
| } | ||
|
|
||
| try { | ||
| return JSON.parse(value); | ||
| } catch { | ||
| return null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
const parseJSON = <T>(value: any): T | null => {
if (typeof value !== "string") {
return value as T;
}
try {
return JSON.parse(value) as T;
} catch {
return null;
}
};- 현재 확인했을 때 value의 인자는 어떠한 타입의 값도 들어갈 수 있고, 이를 parse를 하면
제네릭으로 넘겨준 타입으로 반환하는 것으로 작성하면 어떨까요!? - JSON.parse는 반환값이 any기 때문에 타입을 지정해주는게 좋을 것 같습니다!
// as-is
const falseValue = parseJSON(false); // any
const zeroNumberValue = parseJSON(0); // any
// to-be
const falseValue = parseJSON<false>(false); // false | null
const zeroNumberValue = parseJSON<0>(0); // 0 | nullThere was a problem hiding this comment.
@ssi02014 안그래도 고민하고있던 부분인데 제안 주신 방법이 좋아보입니다!
해당 부분은 수정해두겠습니다! 감사합니다! 👍
There was a problem hiding this comment.
추가적으로 빈 문자열은 그냥 빈 문자열이 반환하는 부분에 대해서는 어떨까요!?
빈 문자열이 null로 반환되기 보다 동일하게 빈 문자열로 반환되는 것이 사용성에 더 낫지 않을까 생각이드네요! 🤔
if (value === "") {
return "";
}There was a problem hiding this comment.
@ssi02014 좋은 의견 감사합니다! 👍
해당 케이스를 보완하면서 에러가 발생하는 케이스에서는 dev환경일 경우 콘솔에러가 찍히는 것이 더 좋을듯 하여 추가하였고, 사용케이스에 제네릭을 추가해두었습니다!
한가지 고민인 부분은 테스트코드에 제네릭에 대한 추론이 잘 적용되었는지에 대한 타입 테스트도 추가해야할지 고민이네요!
해당 함수가 types에 있는 코드처럼 타입을 테스트하는 것이 아닌 유틸로서의 목적을 가지고 정상 동작하는지를 테스트하는 것으로 판단하여 일단 제외해두었습니다!
export const parseJSON = <T>(value: any): T | null => {
if (value === '') {
return '' as T;
}
if (typeof value !== 'string') {
return value as T;
}
try {
return JSON.parse(value) as T;
} catch {
if (process.env.NODE_ENV === 'development') {
console.error(`데이터를 파싱하는 데에 실패했습니다. 원본: ${value}`);
}
return null;
}
};const falseValue = parseJSON<false>(false);
const zeroNumberValue = parseJSON<0>(0);
const emptyStringValue = parseJSON<''>('');There was a problem hiding this comment.
@Sangminnn 타입 테스트 코드는 추가해도 좋고, 작성하지 않아도 낫배드 같습니다! 만약 추가를 원하신다면 아래와 같이 작성하시면 될 것 같습니다 🙏🙏🙏
const falseValue = parseJSON<false>(false);
expectTypeOf(falseValue).toEqualTypeOf<false | null>();console은 NODE_ENV를 검증할 필요까지는 없을 것 같습니다!
리액트의 CRA와 같이 프레임워크 혹은 라이브러리 환경에서는 자동으로 주입해주지만, 단순 html,css, 순수 바닐라 js로 구성된 경우(혹은 직접 커스텀 wepback 환경 -> webpack EnviromentPlugin으로 직접 주입해줘야 함)도 있을 것 같은데 이런 경우에는 �해당 코드는 크게 의미가 없을 것 같아서요!
차라리 조건식은 제거하고 console만 남겨놓는 부분이 더 나을 것 같습니다! 아니면 console을 그냥 제거해도 저는 괜찮습니다👍
추가적으로 아래 빈 문자열 검증 부분은 typeof value 로직보다 아래로 넣는건 어떨까요? string일 경우 검증하는게 좀 더 효율적일 것 같습니다!
string이 아니면 typeof value 부분에서 바로 리턴해주는게 좋을 것 같습니다 !! 👍
if (typeof value !== 'string') {
return value as T;
}
if (value === '') {
return '' as T;
}There was a problem hiding this comment.
@ssi02014 오오 디테일이 살짝 부족했는데 감사합니다! 😂
타입에 대한 검증은 일단 추가하지 않았고, process.env 조건식 제거 및 console만 남겨두었으며 empty string에 대한 검증은 string타입 체크구문 아래로 이동해두었습니다! 👍
docs/docs/utils/common/parseJson.md
Outdated
| const normalObject = parseJSON<NormalObject>(`{ "a": 1, "b": 2 }`); // { a: 1, b: 2 } | ||
| const emptyString = parseJSON<''>(''); // null | ||
| const nullValue = parseJSON<null>(null); // null | ||
| const undefinedValue = parseJSON<undefined>(undefined); // undefined | ||
| const NaNValue = parseJSON<typeof NaN>(NaN); // null |
There was a problem hiding this comment.
| const normalObject = parseJSON<NormalObject>(`{ "a": 1, "b": 2 }`); // { a: 1, b: 2 } | |
| const emptyString = parseJSON<''>(''); // null | |
| const nullValue = parseJSON<null>(null); // null | |
| const undefinedValue = parseJSON<undefined>(undefined); // undefined | |
| const NaNValue = parseJSON<typeof NaN>(NaN); // null | |
| const normalObject = parseJSON<NormalObject>(`{ "a": 1, "b": 2 }`); // { a: 1, b: 2 } | null | |
| const emptyString = parseJSON<''>(''); // '' | null | |
| const nullValue = parseJSON<null>(null); // null | |
| const undefinedValue = parseJSON<undefined>(undefined); // undefined | null | |
| const NaNValue = parseJSON<typeof NaN>(NaN); // number | null |
- 주석 결과가 변경되어야 할 것 같습니다!
There was a problem hiding this comment.
@ssi02014 문서 수정도 해야함을 인지하고 수정했었는데 아래는 순간적으로 타입 반환값이 아니라 값의 반환으로 생각해서 놓쳤네요 😂 감사합니다! 👍
There was a problem hiding this comment.
음 이 부분 제가 착각했네요..!
말씀주신 것 처럼 반환값에대한 주석이 맞는 것 같습니다😭😭
이부분은 제가 #117 에서 수정하도록하겠습니다
There was a problem hiding this comment.
@ssi02014 헉 그렇군요! 확인해주셔서 감사합니다! 👍👍👍
| try { | ||
| return JSON.parse(value) as T; | ||
| } catch { | ||
| console.error(`데이터를 파싱하는 데에 실패했습니다. 원본: ${value}`); |
There was a problem hiding this comment.
한국어로 작성되서 고민을 해봤는데요 modern-kit은 영어를 타겟으로 하지 않고 있어서 추후에 기존에 영어로 작성된 내용들을 한국어로 변환할지 고민이네요..!
ㅎㅎㅎ참고만해주시면 감사드립니다!
There was a problem hiding this comment.
@ssi02014 이전에 말씀해주셨던듯하여 일단 한글로 작성해두었는데 혹시 영어를 타겟으로 하려 하신다면 말씀주시면 언제든 수정하겠습니다!
Overview
일반적으로 JSON.parse를 사용하는 경우 일부 input값에 대해서는 에러를 반환합니다.
이 함수를 사용하면 에러를 반환하는 상황에서 null을 반환하여 예상치 못한 에러상황을 방지할 수 있을 것으로 기대합니다!
편하게 의견주시면 감사드리겠습니다 :)
PR Checklist
Contributing Guide