Standardize timestamp creation #8892
Conversation
| @@ -127,30 +123,6 @@ export function validateRegistryId(id: string | undefined): RegistryId { | |||
| throw new Error("Invalid registry id"); | |||
| } | |||
|
|
|||
| function isTimestamp(value: string): value is Timestamp { | |||
There was a problem hiding this comment.
Moved to timeUtils
Playwright test results
Details
Flaky testsedgeSetup › setup/unaffiliated.setup.ts › authenticate with unaffiliated user Skipped testschrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor |
| @@ -223,14 +223,14 @@ type ActivatedModComponentBase = { | |||
| * | |||
| * Currently, not used for anything - might be used for sorting, etc. in the future. | |||
| */ | |||
| createTimestamp: string; | |||
| createTimestamp: Timestamp; | |||
There was a problem hiding this comment.
Refine type for upcoming PRs
| * @since 2.0.6 | ||
| * @see timestampFactory | ||
| */ | ||
| export function nowTimestamp(): Timestamp { |
There was a problem hiding this comment.
Method to generate timestamps with the Timestamp tagged type
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8892 +/- ##
==========================================
+ Coverage 74.24% 74.41% +0.17%
==========================================
Files 1332 1339 +7
Lines 40817 41180 +363
Branches 7634 7707 +73
==========================================
+ Hits 30306 30646 +340
- Misses 10511 10534 +23 ☔ View full report in Codecov by Sentry. |
| createTimestamp: timestamp, | ||
| updateTimestamp: timestamp, |
There was a problem hiding this comment.
This is the second time I've seen us setting timestamps in the Extension. Assuming these values are persisted, why are we not relying on the server to set the timestamps and return them in a response?
There was a problem hiding this comment.
This is the second time I've seen us setting timestamps in the Extension. Assuming these values are persisted, why are we not relying on the server to set the timestamps and return them in a response?
You are correct - IIRC, these were originally introduced to detect if the activated standalone mod components was outdated (i.e., because it was activated in a different profile). For mod definitions, there's considerations on whether timestamps should be used to determine if the mod is out of date vs. solely relying on the version. (Which the version number might not get updated on save)
The timestamps ARE stored on the server (see the PR discussion on #8891)
Eventually we should not be setting these timestamps. I think this should be handled in the next phase (see the PR discussion on #8891). There's some gotchas around which server response types actually include the timestamps on them
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
ActivatedModComponentBaseFor more information on our expectations for the PR process, see the
code review principles doc