-
Couldn't load subscription status.
- Fork 653
refactor OData date handling and filter operations for better type safety and performance #31382
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
base: 25_2
Are you sure you want to change the base?
refactor OData date handling and filter operations for better type safety and performance #31382
Conversation
…a date handling and filter operations for better type safety and performance
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.
Pull Request Overview
This pull request refactors OData date handling and filter operations to improve type safety and code readability. The changes focus on modernizing JavaScript patterns, adding TypeScript type annotations, and simplifying conditional logic.
Key changes:
- Modernized date formatting utilities by replacing manual string padding with native
padStart/padEndmethods - Added null safety checks and improved error handling in OData date parsing
- Enhanced type annotations across OData utility functions for better type safety
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
m_filter_custom_operations.ts |
Replaced logical AND operator with optional chaining and improved conditional formatting |
m_columns_controller.ts |
Added blank line for code formatting consistency |
m_utils.ts |
Replaced chained if-else with early returns using optional chaining for cleaner control flow |
m_utils.ts (OData) |
Refactored date utilities with TypeScript types, replaced manual padding with native methods, added null check in date parsing, and improved V2 serialization |
m_request_dispatcher.ts |
Added nullish coalescing operator for default value assignment |
| const [dateChunk, timeChunk] = isoString.replace('Z', '').split('T'); | ||
| const date = /(\d{4})-(\d{2})-(\d{2})/.exec(dateChunk); | ||
| const time = /(\d{2}):(\d{2}):(\d{2})\.?(\d{0,7})?/.exec(timeChunk); |
Copilot
AI
Oct 22, 2025
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.
Destructuring assignment may fail if split('T') returns only one element (date without time). timeChunk will be undefined, causing the regex exec on line 74 to fail. Consider handling the case where timeChunk is undefined or use a safer destructuring pattern.
| const [dateChunk, timeChunk] = isoString.replace('Z', '').split('T'); | |
| const date = /(\d{4})-(\d{2})-(\d{2})/.exec(dateChunk); | |
| const time = /(\d{2}):(\d{2}):(\d{2})\.?(\d{0,7})?/.exec(timeChunk); | |
| const splitChunks = isoString.replace('Z', '').split('T'); | |
| const dateChunk = splitChunks[0]; | |
| const timeChunk = splitChunks.length > 1 ? splitChunks[1] : undefined; | |
| const date = /(\d{4})-(\d{2})-(\d{2})/.exec(dateChunk); | |
| const time = typeof timeChunk === 'string' ? /(\d{2}):(\d{2}):(\d{2})\.?(\d{0,7})?/.exec(timeChunk) : null; |
| // OData v4 format | ||
| } else if (ISO8601_DATE_REGEX.test(value)) { | ||
| obj[key] = new Date(parseISO8601(obj[key]).valueOf()); | ||
| obj[key] = new Date(parseISO8601(obj[key])!.valueOf()); |
Copilot
AI
Oct 22, 2025
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.
Non-null assertion operator is unsafe here. If parseISO8601 returns null (which can happen when date regex fails), this will throw a runtime error. Use optional chaining with a fallback or proper null check instead: const parsed = parseISO8601(obj[key]); if (parsed) { obj[key] = new Date(parsed.valueOf()); }
| obj[key] = new Date(parseISO8601(obj[key])!.valueOf()); | |
| const parsed = parseISO8601(obj[key]); | |
| if (parsed) { | |
| obj[key] = new Date(parsed.valueOf()); | |
| } |
| if (typeof value === 'string' && ISO8601_DATE_REGEX.test(value)) { | ||
| return formatISO8601(new Date(value), false, false); | ||
| } |
Copilot
AI
Oct 22, 2025
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.
This new check in serializeValueV2 may create unintended side effects. Any string matching ISO8601 format will be re-formatted, potentially changing values that were intentionally formatted differently. Consider if this behavior is desired or if it should only apply to Date objects.
| if (typeof value === 'string' && ISO8601_DATE_REGEX.test(value)) { | |
| return formatISO8601(new Date(value), false, false); | |
| } | |
| // Removed ISO8601 string re-formatting to avoid unintended side effects. |
- Updated `fieldTypes` and `keyType` in `data.d.ts` to include 'DateTime' and 'DateTimeOffset'. - Improved `keyConverters` in `m_utils.ts` to handle DateTime conversion from string and number types, with error handling for invalid values.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
6533e7a to
959572a
Compare
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.
Pull Request Overview
Copilot reviewed 5 out of 7 changed files in this pull request and generated 1 comment.
| const parseISO8601 = (isoString: string) => { | ||
| const result = new Date(new Date(0).getTimezoneOffset() * 60 * 1000); | ||
| const chunks = isoString.replace('Z', '').split('T'); | ||
| const date = /(\d{4})-(\d{2})-(\d{2})/.exec(chunks[0]); | ||
| const time = /(\d{2}):(\d{2}):(\d{2})\.?(\d{0,7})?/.exec(chunks[1]); | ||
| // @ts-expect-error | ||
| const [dateChunk, timeChunk] = isoString.replace('Z', '').split('T'); | ||
| const date = /(\d{4})-(\d{2})-(\d{2})/.exec(dateChunk); | ||
| const time = /(\d{2}):(\d{2}):(\d{2})\.?(\d{0,7})?/.exec(timeChunk); | ||
|
|
||
| if (!date) { | ||
| return null; | ||
| } |
Copilot
AI
Oct 27, 2025
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.
The function's return type should be explicitly declared as Date | null to match the null return on line 77. This improves type safety for consumers of this function.
…a date handling and filter operations for better type safety and performance