Optimize isSameDay #415
Optimize isSameDay #415
Conversation
| @@ -2,5 +2,9 @@ import moment from 'moment'; | |||
|
|
|||
| export default function isSameDay(a, b) { | |||
| if (!moment.isMoment(a) || !moment.isMoment(b)) return false; | |||
| return a.isSame(b, 'day'); | |||
| // Compare least significant, most likely to change units first | |||
| // Moment's isSame clones moment inputs and is a tad slow | |||
There was a problem hiding this comment.
This seems like something we could PR a fix for into moment itself rather than hacking around it?
There was a problem hiding this comment.
Well I don't think our usecase applies to moment completely because we throw away time. It also seems like given the mutability of moment objects, it's reasonable for them to clone them.
I think this is a good solution for our own usecase.
There was a problem hiding this comment.
By passing "day" we're telling moment to throw away time, so for the "day" code path it seems like it would be an improvement for them as well.
| @@ -15,6 +15,15 @@ describe('isSameDay', () => { | |||
| expect(isSameDay(today, tomorrow)).to.equal(false); | |||
| }); | |||
|
|
|||
| it('returns false for same days of week', () => { | |||
| // Flags accidentally use of moment's day() function, which returns index | |||
There was a problem hiding this comment.
Does this mean that the function had a bug previously? Or does it mean that "is same day" isn't the right semantic, and we want "is same date"? Are we sure that every use of isSameDay wanted "same date" and not "same day"?
Either way, if we want "same date" semantics, we need to rename the function, since "same day" means "same day of week" in JS date parlance.
There was a problem hiding this comment.
We use isSameDay to mean the same day, period, not the same day of the week. moment thinks that date referred to day of month and day referred to day of week which I don't entirely agree with semantically and find confusing (especially since isSame(foo, 'day') does what you'd expect in moment).
The first attempt at improving the speed of this function used moment's day() instead of date() which was a bug, but that got reverted immediately before a release was even made.
This function has literally been called isSameDay for 2 years. I really don't think this PR is the place to update the name if we choose to go that route.
There was a problem hiding this comment.
Ah ok, so there's never been a release with "is same day" semantics, only "is same date"?
In that case it's just a bad name for the function, but that can be changed later, and this code looks fine.
There was a problem hiding this comment.
This spec is to guard against a bug I recently introduced and reverted. Moment is inconsistent about day vs date, and I think our naming makes sense.
There was a problem hiding this comment.
The naming in JS Dates is "day" means "day of week" and "date" means "calendar date" - if moment or we deviate from that, it's a problem that needs fixing.
to: @majapw @lencioni
Implements the
isSameDayoptimizations @lencioni recommended by comparing the most likely to change units first. When I checked that benchmark script this morning, the old version spent 140ms checkingisSameDay(..., today), new version 30ms.The today checks were more expensive than start/end date (even though they both use
isSameDay) because the latter two can shortcircuit when null (today is never null). Moment'sisSameis expensive because it clones the input dates.