ref(utils): Clean up timestamp calculation code #10069
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AbhiPrasad
requested review from
timfish,
anonrig,
a team and
stephanie-anderson
and removed request for
a team
AbhiPrasad
commented
Jan 4, 2024
| @@ -148,7 +90,7 @@ export const browserPerformanceTimeOrigin = ((): number | undefined => { | |||
| // performance.timing.navigationStart, which results in poor results in performance data. We only treat time origin | |||
| // data as reliable if they are within a reasonable threshold of the current time. | |||
|
|
|||
| const { performance } = WINDOW; | |||
| const { performance } = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window; | |||
| if (!performance || !performance.now) { | |||
| _browserPerformanceTimeOriginMode = 'none'; | |||
There was a problem hiding this comment.
Ideally what we do with browserPerformanceTimeOrigin is to make this timeOrigin calculation more generic for browser/node, I generally think it uses a good heuristic to determine timeOrigin reliability.
size-limit report 📦
|
anonrig
reviewed
Jan 4, 2024
| @@ -148,7 +90,7 @@ export const browserPerformanceTimeOrigin = ((): number | undefined => { | |||
| // performance.timing.navigationStart, which results in poor results in performance data. We only treat time origin | |||
| // data as reliable if they are within a reasonable threshold of the current time. | |||
|
|
|||
| const { performance } = WINDOW; | |||
| const { performance } = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window; | |||
There was a problem hiding this comment.
Can we make this a new type instead of this type cast?
There was a problem hiding this comment.
I think it's fine to keep esp because it's only being used here.
mydea
reviewed
Jan 5, 2024
timfish
approved these changes
Jan 5, 2024
c298lee
pushed a commit
that referenced
this pull request
Jan 9, 2024
This PR removes the usage of `dynamicRequire` from `packages/utils/src/time.ts` and cleans up our timestamp code to be simpler and reduce bundle size. Removing `dynamicRequire` means that we no longer rely on `perf_hooks` for the `performance` API, but instead try to grab it from `globalThis.performance`. `performance` was added to the global in Node 16, which means we'll fallback to using `Date.now()` in Node 8, 10, 12, 14 (and in v8 just Node 14). I think that is an acceptable tradeoff, we just reduce accuracy for those versions. This does not refactor `browserPerformanceTimeOrigin` code at the bottom of the file, I want to come back and touch that in a follow up PR to reduce the amount of changes made here. I would appreciate reviews/opinions on this, I'm not 100% confident in the changes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ref #10046
This PR removes the usage of
dynamicRequirefrompackages/utils/src/time.tsand cleans up our timestamp code to be simpler and reduce bundle size. RemovingdynamicRequiremeans that we no longer rely onperf_hooksfor theperformanceAPI, but instead try to grab it fromglobalThis.performance.performancewas added to the global in Node 16, which means we'll fallback to usingDate.now()in Node 8, 10, 12, 14 (and in v8 just Node 14). I think that is an acceptable tradeoff, we just reduce accuracy for those versions.This does not refactor
browserPerformanceTimeOrigincode at the bottom of the file, I want to come back and touch that in a follow up PR to reduce the amount of changes made here.I would appreciate reviews/opinions on this, I'm not 100% confident in the changes.