feat: support convert milliseconds and fix timezone #66
Conversation
…s to multiple different date formats and fixed other two test cases assertions
WalkthroughThe pull request introduces enhanced date manipulation utilities in the project. A new function Changes
Assessment against linked issues
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/date.ts (1)
189-194: Use Epoch constants for better clarity.
Using1000as a magic number is common, but consider referencing a constant likeMILLISECONDS_IN_SECOND = 1000to improve code readability.export function dateToUnixTimestamp(date: Date): number { - return Math.round(date.getTime() / 1000); + const MILLISECONDS_IN_SECOND = 1000; + return Math.round(date.getTime() / MILLISECONDS_IN_SECOND); }README.md (1)
179-179: Add clarifying note about timezones.
Include a small note about how the local timezone affects these date conversion examples. This helps users avoid confusion when comparing local results to the examples shown.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/date.ts(2 hunks)test/date.test.ts(3 hunks)
🔇 Additional comments (8)
src/date.ts (3)
5-11: Ensure negative hour offsets are formatted correctly.
The implementation correctly handles minute offsets by appending them to the hour offset. However, be sure to test negative offsets thoroughly (e.g., UTC-03:30). Some timezones have half or quarter-hour offsets. Consider adding unit tests to guarantee correctness for partial offsets (e.g., UTC+05:30).
196-201: Enum naming is clear and robust.
The enumeration values are appropriately descriptive, making it easy to understand the expected output format.
203-219: Add input validation for large or negative milliseconds.
While the function logic is correct, consider validating against negative or extremely large millisecond values and ensuring they result in a properly formatted date string.test/date.test.ts (4)
87-87: Regex coverage is sufficient.
The new regex pattern looks good, covering a broad range of valid timestamps. Confirm that boundary cases (e.g., single-digit months/days) produce matching results.
195-195: Improve match group coverage for partial-hour offsets.
Currently, the regex allows for offsets up to ±13 hours. If you need to handle offsets beyond ±13 hours or half-hour offsets, broaden or refine the regex.
242-248: Validate DST transitions.
When converting a known UTC date to a Unix timestamp, ensure you test Daylight Saving Time transitions if the environment or test suite simulates them.
250-282: Great test coverage forgetDateFromMilliseconds().
Multiple formats are tested thoroughly, which is excellent. If possible, add a test for edge cases like Unix epoch start (0 ms) or a negative timestamp.README.md (1)
180-185: Great documentation forgetDateFromMilliseconds().
Providing multiple usage samples for the new function is highly beneficial. This clarifies behavior for each format.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/date.ts (2)
1-8: Consider renaming the function for clarity.
The functionresetTimezonedoesn't actually reset anything; it merely constructs and returns a timezone offset string. A more descriptive name likegetTimezoneOffsetStringmight be clearer.
193-198: Consider adding documentation for each enum value.
Providing brief inline comments for eachDateFormatmember can improve clarity for new contributors.test/date.test.ts (1)
87-87: Regex pattern might be more readable.
Consider moving the$symbol outside the capturing group for clarity, e.g.([0-5][0-9])$instead of([0-5][0-9]$).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/date.ts(3 hunks)test/date.test.ts(3 hunks)
🔇 Additional comments (6)
src/date.ts (3)
59-59: No concerns at this line.
The use of theresetTimezonefunction here is consistent.
185-191: Implementation is correct.
This function accurately converts aDateobject to a Unix timestamp in seconds. Keep in mind that any fractional milliseconds are rounded.
200-216: Meets the PR objective.
This function directly addresses the request in #35 by formatting a date derived from milliseconds into various output styles.test/date.test.ts (3)
195-195: Consider UTC+14 edge cases.
If you need to accommodate regions using UTC+14, update(0[0-9]|1[0-3])to include14.
241-248: Test coverage fordateToUnixTimestampis thorough.
These assertions properly validate the conversion to a Unix timestamp in seconds.
250-281: Comprehensive testing ofgetDateFromMilliseconds.
All expected formats are covered, ensuring robust validation for eachDateFormatcase.
commit: |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/date.ts (2)
4-12: Rename function to better reflect its purposeThe function name
resetTimezonedoesn't accurately describe its purpose. It's actually formatting a timezone offset string.Consider renaming:
- export function resetTimezone(date: Date) { + export function formatTimezoneOffset(date: Date) {
204-227: Make the default format explicitThe function falls back to YYYYMMDD format when no format is specified, but this behavior is implicit.
Consider making it explicit:
export enum DateFormat { DateTimeWithTimeZone, DateTimeWithMilliSeconds, DateTimeWithSeconds, - UnixTimestamp + UnixTimestamp, + YYYYMMDD // Add explicit default format } export function getDateFromMilliseconds(milliseconds: number, format?: DateFormat): string { switch (format) { case DateFormat.DateTimeWithTimeZone: return accessLogDate(new Date(milliseconds)); case DateFormat.DateTimeWithMilliSeconds: return logDate(new Date(milliseconds)); case DateFormat.DateTimeWithSeconds: return YYYYMMDDHHmmss(new Date(milliseconds)); case DateFormat.UnixTimestamp: return dateToUnixTimestamp(new Date(milliseconds)).toString(); + case DateFormat.YYYYMMDD: + return YYYYMMDD(new Date(milliseconds)); default: - return YYYYMMDD(new Date(milliseconds)); + return YYYYMMDD(new Date(milliseconds)); // Default to YYYYMMDD format } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(1 hunks)src/date.ts(3 hunks)
🔇 Additional comments (1)
src/date.ts (1)
197-202: LGTM!The implementation correctly converts a Date object to a Unix timestamp in seconds.
fengmk2
changed the title
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/date.ts (1)
66-73:⚠️ Potential issueCritical: Implement the approved timezone caching fix
The current implementation still has the issues identified in the previous review:
- Using static 'TIMEZONE' key means all dates share the same cached timezone
- 100-day TTL is too long considering DST changes
Please implement the previously approved fix:
export function getTimezone(d: Date) { - const timeZone = lru.get('TIMEZONE'); + const key = d.getFullYear() + '-' + (d.getMonth() + 1) + '-' + d.getDate(); + const timeZone = lru.get(key); if (timeZone === undefined) { - lru.set('TIMEZONE', resetTimezone(d), { maxAge: 8640000 }); + lru.set(key, resetTimezone(d), { maxAge: 86400000 }); // Cache for 24 hours return lru.get(key); } return timeZone; }
🧹 Nitpick comments (2)
src/date.ts (2)
4-12: Consider adding type annotations for better type safetyThe timezone calculation logic is correct and handles both positive and negative offsets properly. However, consider adding type annotations for better type safety.
- let TIMEZONE = ''; + let TIMEZONE: string = ''; - const _hourOffset = Math.floor(-offsetInMinutes / 60); - const _minuteOffset = Math.abs(offsetInMinutes % 60); + const _hourOffset: number = Math.floor(-offsetInMinutes / 60); + const _minuteOffset: number = Math.abs(offsetInMinutes % 60);
214-227: Add input validation and explicit default formatWhile the implementation is good, consider these improvements:
- Add input validation for milliseconds
- Make the default format case explicit using DateFormat enum
export function getDateFromMilliseconds(milliseconds: number, format?: DateFormat): string { + if (!Number.isFinite(milliseconds)) { + throw new Error('Invalid milliseconds value'); + } switch (format) { case DateFormat.DateTimeWithTimeZone: return accessLogDate(new Date(milliseconds)); case DateFormat.DateTimeWithMilliSeconds: return logDate(new Date(milliseconds)); case DateFormat.DateTimeWithSeconds: return YYYYMMDDHHmmss(new Date(milliseconds)); case DateFormat.UnixTimestamp: return dateToUnixTimestamp(new Date(milliseconds)).toString(); default: - return YYYYMMDD(new Date(milliseconds)); + // Default to simple date format + const defaultFormat = DateFormat.DateTimeWithSeconds; + return getDateFromMilliseconds(milliseconds, defaultFormat); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/date.ts(3 hunks)
🔇 Additional comments (3)
src/date.ts (3)
1-2: LGTM: LRU cache initializationThe cache size of 1000 entries is appropriate and matches the previously approved configuration.
62-62: LGTM: Proper timezone handlingGood improvement using the new
getTimezonefunction for dynamic timezone calculation.
200-202: LGTM: Unix timestamp conversionClean and efficient implementation of the Unix timestamp conversion.
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/date.ts (1)
215-232: Consider adding format examples in JSDocWhile the implementation is correct, it would be helpful to add examples of the output format for each DateFormat option in the JSDoc comments.
Add documentation like this:
/** * Provide milliseconds, return a formatted string. + * @example + * // DateTimeWithTimeZone: "16/Apr/2013:16:40:09 +0800" + * // DateTimeWithMilliSeconds: "2013-04-16 16:40:09.123" + * // DateTimeWithSeconds: "2013-04-16 16:40:09" + * // UnixTimestamp: "1366098009" + * // Default: "2013-04-16" */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/date.ts(3 hunks)
🔇 Additional comments (5)
src/date.ts (5)
1-2: LGTM: Cache initialization looks good!The LRU cache is properly initialized with a reasonable size of 1000 entries.
4-12: LGTM: Timezone calculation looks accurate!The timezone calculation correctly handles:
- Both positive and negative offsets
- Hour and minute components
- Proper padding of numbers
66-74: LGTM: Timezone caching implementation follows best practices!The implementation:
- Uses date-specific cache keys to handle DST changes
- Has appropriate TTL (24 hours)
- Properly handles cache misses
201-203: LGTM: Unix timestamp conversion is accurate!The implementation correctly converts milliseconds to seconds and handles rounding.
205-210: LGTM: DateFormat enum provides good format options!The enum covers common date format requirements:
- Timezone-aware format
- Millisecond precision
- Second precision
- Unix timestamp
use `as const` instead of enum
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/date.ts (2)
201-203: Add input validation.Consider validating the input date to handle null/invalid dates gracefully.
export function dateToUnixTimestamp(date: Date): number { + if (!(date instanceof Date) || isNaN(date.getTime())) { + throw new Error('Invalid Date object'); + } return Math.round(date.getTime() / 1000); }
217-234: Enhance type safety.Consider improving type safety with a more specific return type and type guard for the format parameter.
+type DateFormatType = typeof DateFormat[keyof typeof DateFormat]; + -export function getDateFromMilliseconds(milliseconds: number, format?: DateFormat): string { +export function getDateFromMilliseconds( + milliseconds: number, + format?: DateFormatType +): string { if (!Number.isFinite(milliseconds)) { throw new Error('Invalid milliseconds value'); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/date.ts(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/date.ts
[error] 207-207: Const declarations must have an initialized value.
This variable needs to be initialized.
(parse)
[error] 207-207: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 209-209: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 212-212: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 212-212: Expected an identifier, an array pattern, or an object pattern but instead found ';'.
Expected an identifier, an array pattern, or an object pattern here.
(parse)
[error] 208-208: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 207-212: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (4)
src/date.ts (4)
1-2: LGTM! Cache size is properly configured.The LRU cache initialization with 1000 entries follows the previous recommendations.
4-12: LGTM! Robust timezone offset calculation.The implementation correctly handles:
- Both positive and negative offsets
- Proper padding for hours and minutes
- Local scope for TIMEZONE variable
62-62: LGTM! Consistent usage of cached timezone.The change properly integrates with the new timezone caching mechanism.
66-74: LGTM! Well-implemented timezone caching.The implementation follows best practices:
- Date-based cache keys for proper DST handling
- Appropriate 24-hour cache duration
- Proper cache miss handling
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/date.ts (2)
201-203: Add input validation and handle edge casesThe function should validate input and handle edge cases like invalid dates.
Consider this enhanced implementation:
export function dateToUnixTimestamp(date: Date): number { + if (!(date instanceof Date) || isNaN(date.getTime())) { + throw new Error('Invalid Date object'); + } return Math.round(date.getTime() / 1000); }
214-234: Enhance documentation and type safetyThe function implementation could be improved in several ways:
- Add JSDoc for the format parameter
- Use type narrowing for better type safety
- Consider using a Map for constant-time format lookup
Consider these improvements:
/** * Provide milliseconds, return a formatted string. + * @param milliseconds - The number of milliseconds since Unix epoch + * @param format - Optional format for the output date string + * @returns A formatted date string based on the specified format */ export function getDateFromMilliseconds(milliseconds: number, format?: DateFormat): string { if (!Number.isFinite(milliseconds)) { throw new Error('Invalid milliseconds value'); } + const formatters = new Map([ + [DateFormat.DateTimeWithTimeZone, (date: Date) => accessLogDate(date)], + [DateFormat.DateTimeWithMilliSeconds, (date: Date) => logDate(date)], + [DateFormat.DateTimeWithSeconds, (date: Date) => YYYYMMDDHHmmss(date)], + [DateFormat.UnixTimestamp, (date: Date) => dateToUnixTimestamp(date).toString()], + ]); + + const date = new Date(milliseconds); + const formatter = format ? formatters.get(format) : null; + return formatter ? formatter(date) : YYYYMMDD(date); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/date.ts(3 hunks)
🔇 Additional comments (2)
src/date.ts (2)
205-212: Well-structured format definitionsGood use of 'as const' assertion for type safety and clear, descriptive format names.
66-74: Consider edge case with timezone changes at midnightThe current implementation caches timezone per day, but timezone transitions (like DST changes) typically occur at specific times (often 2 AM). A date near midnight might get cached with the wrong offset if the transition occurs in the next few hours.
Consider this alternative implementation:
export function getTimezone(d: Date) { - const key = d.getFullYear() + '-' + (d.getMonth() + 1) + '-' + d.getDate(); + const key = d.getFullYear() + '-' + (d.getMonth() + 1) + '-' + d.getDate() + '-' + d.getHours(); const timeZone = lru.get(key); if (timeZone === undefined) { lru.set(key, resetTimezone(d), { maxAge: 86400000 }); // Cache for 24 hours return lru.get(key); } return timeZone; }
| } | ||
|
|
||
| export enum DateFormat { | ||
| DateTimeWithTimeZone = 'DateTimeWithTimeZone', |
There was a problem hiding this comment.
Change enum value to string is better than number.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #66 +/- ##
==========================================
+ Coverage 99.12% 99.16% +0.03%
==========================================
Files 13 13
Lines 917 961 +44
Branches 188 199 +11
==========================================
+ Hits 909 953 +44
Misses 8 8 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/date.ts (1)
4-12: Add JSDoc documentation for the resetTimezone function.The function implementation looks good, but it would benefit from documentation explaining the timezone format and the meaning of the return value.
Add this documentation:
+/** + * Calculates timezone offset string in ±HHMM format for the given date + * @param date - The date to calculate timezone offset for + * @returns Timezone offset string (e.g., '+0800' for UTC+8, '-0530' for UTC-5:30) + */ export function resetTimezone(date: Date) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/date.ts(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/date.ts
[warning] 217-218: src/date.ts#L217-L218
Added lines #L217 - L218 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Node.js / Test (windows-latest, 23)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (windows-latest, 16)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (macos-latest, 18)
🔇 Additional comments (5)
src/date.ts (5)
1-2: LGTM! Cache initialization looks good.The LRU cache is properly initialized with an appropriate size of 1000 entries.
66-74: LGTM! Timezone caching implementation is solid.The implementation correctly:
- Uses date-based cache keys to handle DST changes
- Sets appropriate TTL (24 hours)
- Handles cache misses properly
201-203: LGTM! Unix timestamp conversion looks good.The implementation correctly converts Date to Unix timestamp in seconds.
205-210: LGTM! DateFormat enum is well-defined.The enum provides clear and meaningful format options for date conversion.
215-232: Add test coverage and improve input validation.
- The error handling branch (lines 216-218) lacks test coverage
- Consider validating negative milliseconds
- The default case could be more explicit about falling back to YYYYMMDD format
Add this test case to cover error handling:
it('should throw error for invalid milliseconds', () => { expect(() => getDateFromMilliseconds(NaN)).toThrow('Invalid milliseconds value'); expect(() => getDateFromMilliseconds(Infinity)).toThrow('Invalid milliseconds value'); });Consider these improvements:
export function getDateFromMilliseconds(milliseconds: number, format?: DateFormat): string { - if (!Number.isFinite(milliseconds)) { + if (!Number.isFinite(milliseconds) || milliseconds < 0) { throw new Error('Invalid milliseconds value'); } switch (format) { case DateFormat.DateTimeWithTimeZone: return accessLogDate(new Date(milliseconds)); case DateFormat.DateTimeWithMilliSeconds: return logDate(new Date(milliseconds)); case DateFormat.DateTimeWithSeconds: return YYYYMMDDHHmmss(new Date(milliseconds)); case DateFormat.UnixTimestamp: return dateToUnixTimestamp(new Date(milliseconds)).toString(); default: + // Default to YYYYMMDD format when no format is specified return YYYYMMDD(new Date(milliseconds)); } }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 217-218: src/date.ts#L217-L218
Added lines #L217 - L218 were not covered by tests
|
@abu-sufyan1 Thanks for your open-source contributions 🎉 |
[skip ci] ## [2.5.0](v2.4.0...v2.5.0) (2025-01-14) ### Features * support convert milliseconds and fix timezone ([#66](#66)) ([4bc6691](4bc6691))
2 participants
close #35 #36
A function is added that takes milliseconds as an argument and returns Date object, if second optional parmeters is provided, it will return the date in the specified format.
Also, the pull request contains a fix other two test cases which were failing earlier.
We put the TIMEZONE object inside the resetTimezone() function, and now we are getting the timezone based on the date that needs to be transformed.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Dateobjects to Unix timestamps.Documentation
Tests
Chores