Fix inter-timezone schedule problems with moment-timezone #19
Conversation
| @@ -0,0 +1,16 @@ | |||
| define(["moment-timezone"], function (moment) { | |||
There was a problem hiding this comment.
You might notice that this is an incomplete data set for the America/Toronto timezone. This is intentional, as it speeds up checking which timezone "rule" applies. We don't care about any dates before 2007, so this should be fine.
There was a problem hiding this comment.
This is a clever solution on the part of Moment to not require loading up a huge data file on the client side.
There was a problem hiding this comment.
Do you mind commenting the URL where you can generate this file? ie. http://momentjs.com/timezone/data/
|
I briefly looked at this, but this LGTM. Thanks for dealing with all this timezone crap!! This is great stuff. |
| @@ -0,0 +1,8 @@ | |||
| define(["moment", "moment-timezone", "ext/moment-timezone-data"], function (_moment, __, __) { | |||
| // Always use America/Toronto as the timezone. | |||
| return function rmc_moment(a, b, c, d) { | |||
There was a problem hiding this comment.
This is cool syntax to avoid leaving anonymous functions nameless. :)
There was a problem hiding this comment.
Yep. Makes it more useful in stack traces and in profiling.
|
Thanks! This is nice, and today I learned about |
| return _moment(a, b, c, d) | ||
| .tz("America/Toronto"); | ||
| } | ||
| rmcMoment.tz = _moment.tz; |
There was a problem hiding this comment.
What do you think about doing a _.extend here to ensure we bring in all the static methods?
There was a problem hiding this comment.
eg. if someone tries to call moment.isMoment they might be confused wondering why it doesn't work....
All of the following should work now: Upload in America/Los_Angeles, view in America/Toronto Upload in America/Toronto, view in America/Toronto Upload in America/Los_Angeles, view in America/Los_Angeles Upload in America/Toronto, view in America/Los_Angeles Also updates to Moment.js 2.4.0
This comes with a huge perf boost due to memoization! See: moment/moment-timezone#39
Fix inter-timezone schedule problems with moment-timezone
|
W00t, Finally rid of that annoying timezone issue. Thanks! |
The way this works is by using the
rmc_momentfile instead ofmomentas a shim to always ensure that the timezone "America/Toronto" is set.Testing steps:
Upload schedule in Tab 1. Schedule should look right.
Reload Tab 2. Schedule should be fine.
Upload schedule in Tab 2. Schedule should look right.
Reload Tab 1. Schedule should be fine.
This unfortunately still adds about 400-500ms to the page load time for me because of the moment-timezone perf problems. I added to an existing issue here: moment/moment-timezone#44 and am going to try to look into myself.
We also need to figure out a sensible way of doing a backfill for this to fix all the schedules uploaded outside of America/Toronto (all the ones inside there should be fine).
This also version bumps moment to 2.4.0, and switches to unminified source. When we compress to send things up to prod, I believe we compress them all anyway - and this made profiling much much easier.