-
Notifications
You must be signed in to change notification settings - Fork 1
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: Add startOfByTimezone and endOfByTimezone functions #231
Conversation
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.
오 이거 좋네요!
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.
LGTM
src/date-util/date-util.ts
Outdated
export function startOfByTimezone( | ||
date: DateType, | ||
property: DatePropertyType, | ||
timezone: TimeZoneType = 'Asia/Seoul' |
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.
P3: 이 default 값은, 제가 예제로 드릴 때는 있었지만, 실제 사용에서는 없는게 좋을것 같아요.
사용하는 쪽에서 코드를 읽을 때 타임존이 명시되어 있는 편이 읽기 좋을것 같거든요.
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.
넵! 읽는 쪽의 가독성을 생각하면 timezone이 명시되는게 좋을 것 같군요
named parameter로 변경하면서 default 값을 제거하도록 하겠습니다!
src/date-util/date-util.ts
Outdated
result.setUTCFullYear(result.getUTCFullYear() + 1); | ||
break; | ||
case 'month': | ||
result.setUTCMonth(result.getUTCMonth() + 2, 0); |
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.
P2: 이것 이상하네요. month 는 원래 0-based index 이긴한데, 여기서 착각하신거 아닌가 확인부탁드려요.
정상동작이라면 저처럼 의문 가진사람이 알 수 있게 주석 추가해주면 좋을 것 같아요.
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.
네 저도 코드 흐름상 이 부분에서 의문이 생길 수 있을 것 같아 주석을 추가할까 했었습니다!
'month' 기준으로 endOf를 구할 때는 startOf의 한달 뒤를 구해야합니다.
따라서 한달을 추가하면 되는데, setUTCMonth
의 두번째 인자는 date를 셋팅해줍니다. 이때 0이 들어가게 되면 전달의 마지막날이 구해지기 때문에 +1에 한번 더 +1을 해주게 되었습니다.
ex) 2024-11-10
- getUTCMonth -> 10
- setUTCMonth(10+2) -> 2024-12-10
- setUTCMonth(12, 0) -> 2024-11-30
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.
그런데, setUTCMonth 의 첫번째 인자는 monthValue (0-based), 두번째 인자는 dayValue (1-based) 여서 두번째 인자를 1로 맞춰주면 되는 문제 아닌가 싶어요.
setUTCMonth(monthValue, dateValue)
dateValue Optional
An integer from 1 to 31 representing the day of the month.
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.
아,, 맞습니다 여기서, 예상하지 않았던 case가 있었네요.
timezone에 맞게 startOf을 실행하면, month 같은 경우는 월이 변경 되는 케이스가 있었습니다.
ex) startOf(2024-02-15Z) -> 2024-01-31 15:00:00Z
그래서 offset 시간 만큼을 더하고 빼주는 방법으로 변경했습니다.
더 깔끔한 방법이 있을까요?
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.
startOf를 아래에 추가해서 offset을 빼주는 과정하나를 제거했습니다..!
다음 날짜로 시간을 변경하긴 해야하는데, 그 기준이 timezone offset이 되어야 아래 케이스를 지원할 수 있을 것 같습니다.
it('15시 이후', () => {
const targetDate = new Date('2024-02-29T16:15:15Z');
const timezone = 'Asia/Seoul';
const result = endOfByTimezone({ date: targetDate, property: 'month', timezone });
expect(result).toEqual(new Date('2024-03-31T15:00:00Z'));
});
it('15시 이전', () => {
const targetDate = new Date('2024-02-29T14:15:15Z');
const timezone = 'Asia/Seoul';
const result = endOfByTimezone({ date: targetDate, property: 'month', timezone });
expect(result).toEqual(new Date('2024-02-29T15:00:00Z'));
});
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.
테스트 통과했군요, 일단 좋네요. 시간 계산이 좀 어려운데, 다른 언어들은 어떻게 하는지 보면서 저도 좀 익혀야할 것 같습니다.
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.
슬랙에서 코멘트 주신부분인데 아래 방법도 좋아 보이네요
export function endOfByTimezone({
date,
property,
timezone,
}: {
date: DateType;
property: DatePropertyType;
timezone: TimeZoneType;
}): Date {
const parsedDate = new Date(date);
switch (property) {
case 'year':
parsedDate.setUTCFullYear(parsedDate.getUTCFullYear() + 1);
break;
case 'month':
// 이 부분을 timezone 없이 new Date를 한다.
const dateWithoutOffset = new Date(parsedDate.toLocaleString('en', { timeZone: timezone }));
parsedDate.setUTCMonth(dateWithoutOffset.getUTCMonth() + 1, 1);
break;
case 'day':
parsedDate.setUTCDate(parsedDate.getUTCDate() + 1);
break;
case 'hour':
parsedDate.setUTCHours(parsedDate.getUTCHours() + 1);
break;
case 'minute':
parsedDate.setUTCMinutes(parsedDate.getUTCMinutes() + 1);
break;
}
const result = startOfByTimezone({ date: parsedDate, property, timezone });
return result;
}
bc474b0
to
cd69c2e
Compare
cd69c2e
to
b65e1e6
Compare
PR 의 종류는 어떤 것인가요?
수정이 필요하게된 이유가 무엇인가요? (Jira 이슈가 있다면 링크를 연결해주세요)
실행 context와 무관하게 특정 시간대의 시작일과 종료일을 리턴하는 메소드가 필요해서 추가합니다.
정산에서 특정 월에 해당하는 시작일과 종료일이 필요한 경우가 있습니다.
ex)
2024년1월
자세한 내용은 테스트 코드에 추가해두었습니다.
무엇을 어떻게 변경했나요?
코드 변경을 이해하기 위한 배경지식이 필요하다면 설명 해주세요.
디펜던시 변경이 있나요?
어떻게 테스트 하셨나요?
테스트 코드
코드의 실행결과를 볼 수 있는 로그나 이미지가 있다면 첨부해주세요.