Conversation
There was a problem hiding this comment.
Pull request overview
This pull request removes the moment and moment-timezone dependencies and replaces them with native JavaScript Date APIs. This is part of a broader effort across Appium projects to reduce dependencies.
Changes:
- Removed moment and moment-timezone from package.json dependencies
- Implemented custom
formatDateandparseTimestampfunctions to replace moment's formatting/parsing - Replaced moment's timezone validation with
Intl.DateTimeFormatAPI - Added comprehensive unit tests for the new time-related functions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| package.json | Removed moment and moment-timezone dependencies |
| lib/commands/time.ts | Replaced moment with custom formatting/parsing functions and Intl API for timezone validation |
| lib/commands/media-projection.ts | Replaced moment formatting with custom getCurrentTimestampFilename function |
| test/unit/commands/time-specs.ts | Added comprehensive unit tests for time-related functions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function formatDate(timestamp: string, format: string): string { | ||
| // Parse the timestamp to get both the date and the timezone offset | ||
| const date = new Date(timestamp); | ||
|
|
||
| let timezoneOffsetMinutes = -date.getTimezoneOffset(); | ||
| const tzMatch = timestamp.match(/([+-])(\d{2}):?(\d{2})$/); | ||
| if (tzMatch) { | ||
| const [, sign, hours, minutes] = tzMatch; | ||
| timezoneOffsetMinutes = (sign === '+' ? 1 : -1) * (parseInt(hours, 10) * 60 + parseInt(minutes, 10)); | ||
| } | ||
|
|
||
| const utcTime = date.getTime(); | ||
| const adjustedTime = new Date(utcTime + timezoneOffsetMinutes * 60000); |
There was a problem hiding this comment.
The formatDate function attempts to parse the timestamp string independently on line 13, but timestamps in the format '2024-01-15T10:30:45+0530' (without colon in timezone offset) are not valid ISO 8601 and may not parse correctly with new Date(). The parseTimestamp function already handles this case correctly by converting the format before parsing, but formatDate doesn't use that parsed result. This function should either accept a Date object as a parameter (in addition to the timestamp string for timezone extraction) or reuse the parseTimestamp logic. The current implementation may produce incorrect results when Date parsing fails on line 13.
| import moment from 'moment-timezone'; | ||
| import type {AndroidDriver} from '../driver'; | ||
|
|
||
| const MOMENT_FORMAT_ISO8601 = 'YYYY-MM-DDTHH:mm:ssZ'; |
There was a problem hiding this comment.
The constant name 'MOMENT_FORMAT_ISO8601' still references 'moment' even though the moment library has been removed. Consider renaming to 'DEFAULT_FORMAT_ISO8601', 'ISO8601_FORMAT', or simply 'FORMAT_ISO8601' to remove the obsolete reference.
| const tzMatch = timestamp.match(/([+-])(\d{2}):?(\d{2})$/); | ||
| if (tzMatch) { | ||
| const [, sign, hours, minutes] = tzMatch; | ||
| timezoneOffsetMinutes = (sign === '+' ? 1 : -1) * (parseInt(hours, 10) * 60 + parseInt(minutes, 10)); |
There was a problem hiding this comment.
The regex pattern on line 16 only matches numeric timezone offsets like '+0530' or '+05:30', but doesn't match the 'Z' timezone indicator (which represents UTC, i.e., +00:00). When a timestamp with 'Z' is processed, the function falls back to using the local timezone offset from line 15 (-date.getTimezoneOffset()), which is the offset of the machine running the code, not the device timezone. This causes incorrect formatting when the device returns a UTC timestamp. The regex should also handle the 'Z' case, or there should be a separate check for it.
| const tzMatch = timestamp.match(/([+-])(\d{2}):?(\d{2})$/); | |
| if (tzMatch) { | |
| const [, sign, hours, minutes] = tzMatch; | |
| timezoneOffsetMinutes = (sign === '+' ? 1 : -1) * (parseInt(hours, 10) * 60 + parseInt(minutes, 10)); | |
| const hasZuluOffset = /Z$/i.test(timestamp); | |
| const tzMatch = timestamp.match(/([+-])(\d{2}):?(\d{2})$/); | |
| if (hasZuluOffset) { | |
| // Explicit UTC indicator ("Z" => +00:00) | |
| timezoneOffsetMinutes = 0; | |
| } else if (tzMatch) { | |
| const [, sign, hours, minutes] = tzMatch; | |
| timezoneOffsetMinutes = | |
| (sign === '+' ? 1 : -1) * (parseInt(hours, 10) * 60 + parseInt(minutes, 10)); |
|
No, this breaks time format's current spec, so let me close |
|
Since this repo has more than one use of |
Inspired with appium/appium#21960