Remove moment as a dependency #458
Conversation
mqp
force-pushed
the
lets-trash-that-out
branch
2 times, most recently
from
b8529d9
to
d2186db
Compare
| @@ -334,7 +333,7 @@ const onReady = async () => { | |||
| document.body.addEventListener("connected", () => { | |||
| if (!isBotMode) { | |||
| hubChannel.sendEntryEvent().then(() => { | |||
| store.update({ activity: { lastEnteredAt: moment().toJSON() } }); | |||
There was a problem hiding this comment.
unfortunately we can't do this, because this is a persistent schema in local storage. we'll need to either create a new field or write something to migrate the old data. (new clients will throw when the json is passed to the Date constructor I presume)
There was a problem hiding this comment.
(my preference is we write a migration script given that this will break our stats otherwise)
There was a problem hiding this comment.
I'm not sure I follow. The new version should be putting the exact same value as the old version into the store, namely a string with the current UTC ISO8601 datetime. The Date constructor knows how to parse that.
(moment().toJSON() sounds funny, but it just means to return an appropriate JSON serialization of a moment value, which is as above. It's meant to be used with JSON.stringify, which calls toJSON on the objects it visits.)
There was a problem hiding this comment.
ah I misunderstood. i assumed it was returning some kind of JSON blob
| lastEnteredPst.dayOfYear() !== nowPst.dayOfYear() || lastEnteredPst.year() !== nowPst.year(); | ||
| entryTimingFlags.isNewMonthly = | ||
| lastEnteredPst.month() !== nowPst.month() || lastEnteredPst.year() !== nowPst.year(); | ||
| entryTimingFlags.isNewDayWindow = lastEntered.isBefore(dayWindowAgo); |
There was a problem hiding this comment.
this seems OK I think -- it changes what we're measuring but we haven't used these fields yet anyhow
mqp
dismissed
gfodor’s stale review
There's no changes to the data in local storage.
This was 400KB of functionality we didn't really need.
The only functionality difference here should be that the
isNewDayWindowandisNewMonthWindowflags are based on fixed intervals of 3600 * 24 and 3600 * 24 * 30 seconds respectively, instead of being one month or day of calendar time. Frankly, this seems better to me for most kinds of analytics anyone would want to do. @gfodor should take a look and decide whether he finds that desirable or undesirable -- we could do it either way if we wanted.