Refactor utils #8360
Refactor utils #8360
Conversation
|
💖 Thanks for opening this pull request! 💖 Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
src/js/utils/buffer.js
Outdated
| if (end > duration) { | ||
| end = duration; | ||
| } | ||
| end = Math.min(end, duration); |
There was a problem hiding this comment.
This tries to redefine a const
There was a problem hiding this comment.
That's unbelievable mistake, I've fixed it.
| @@ -60,19 +60,15 @@ export const bind_ = function(context, fn, uid) { | |||
| * | |||
| * @return {Function} | |||
| */ | |||
| export const throttle = function(fn, wait) { | |||
| let last = window.performance.now(); | |||
There was a problem hiding this comment.
This was changed to use window.performance.now() in #5870. Why do you propose changing it to use Date.now()?
There was a problem hiding this comment.
That's a good question, we use window.performance.now() when we need a high level of accuracy as it is a measurement of floating point milliseconds, but do these 0.153184 milliseconds really matter in our situation? I don't think they matter because we are using 30 milliseconds as a delay for the throttle function which is so huge compared to time % 1, so we can simply neglect these neglectable numbers which have small bad effect on the performance, as window.performance.now() is 3 to 4 times slower than Date.now(), as shown in the following picture:
Finally, I have initialized lastCallTime with 0, to avoid calling any function that requires a non-zero milliseconds to run.
Also paying some attention to popularity and browser compatibility, we will find that Date.now() is the winner.
There was a problem hiding this comment.
The main reason to use performance.now() here isn't the higher resolution but the guarantee of monotonic time updates. I would be inclined to leave it as is due to that.
I'm not sure that the performance difference between Date.now() and performance.now() in this usage are significant enough to worry about.
There was a problem hiding this comment.
Perhaps the method used to determine the start time should be configurable, I can imagine performance.now() being superceded by some other better native JS alternative at some point (it never stops, does it) so a method that allows the user to use Date.now() or performance.now() or even nextnewthing.now(), is a reasonable idea, one I should've thought of when I submitted #5870
That being said, please don't change it back to new Date().getTime()!
| @@ -141,7 +141,7 @@ function createTimeRangesObj(ranges) { | |||
| export function createTimeRanges(start, end) { | |||
| if (Array.isArray(start)) { | |||
| return createTimeRangesObj(start); | |||
| } else if (start === undefined || end === undefined) { | |||
| } if (start === undefined || end === undefined) { | |||
There was a problem hiding this comment.
| } if (start === undefined || end === undefined) { | |
| } | |
| if (start === undefined || end === undefined) { |
No description provided.