feat: be explicit when specifying time & replace moment with date-fns #1072

theneva · 2021-10-27T13:41:10Z

This uses date-fns for all (I hope) fiddling with time and durations.

I think this makes it way easier to understand which units are used (for example, milliseconds or seconds). It also removes the need for things like hard coded millisecond representations of 24 hours.

I took the liberty of replacing all usage of moment with date-fns, and removing the dependency. (I did not, however, update the docs page that uses moment in an example (website/docs/advanced/custom-activation-strategy.md)).

I'll write some inline PR comments in the places where I think the changes are less than obvious – but feel free to ask questions 😄

A small note: I've not been super strict about using just one of Date.now() and new Date(), since it doesn't matter. date-fns happily consumes both.

Do you think this makes sense?

theneva

Wrote some comments to help the poor reader of this all-over-the-place diff.

theneva · 2021-10-27T13:41:50Z

src/lib/middleware/secure-headers.ts


 const secureHeaders: (config: IUnleashConfig) => RequestHandler = (config) => {
    if (config.secureHeaders) {
        return helmet({
            hsts: {
-                maxAge: 63072000,
+                maxAge: hoursToSeconds(24 * 365 * 2), // 2 non-leap years


$ node > require('date-fns').hoursToSeconds(24 * 365 * 2) 63072000

(There is no way to safely translate hours to days, due to daylight savings time. The same goes for days to years, due to leap years. Therefore (I believe), date-fns offers no utilities to make those conversions, and I just kept the original value. I think this is the only inline code comment I wrote.)

theneva · 2021-10-27T13:45:28Z

src/lib/routes/admin-api/state.ts

@@ -88,7 +88,7 @@ class StateController extends Controller {
            includeTags,
            includeEnvironments,
        });
-        const timestamp = moment().format('YYYY-MM-DD_HH-mm-ss');
+        const timestamp = formatDate(Date.now(), 'yyyy-MM-dd_HH-mm-ss');


$ node > require('moment')().format('YYYY-MM-DD_HH-mm-ss') '2021-10-27_15-06-43' > require('date-fns').format(Date.now(), 'yyyy-MM-dd_HH-mm-ss') '2021-10-27_15-07-01'

(The minute and seconds are different because I ran the commands 18 seconds apart.)

theneva · 2021-10-27T13:46:16Z

src/lib/services/addon-service.ts

@@ -75,7 +74,7 @@ export default class AddonService {
            async () => addonStore.getAll({ enabled: true }),
            {
                promise: true,
-                maxAge: ADDONS_CACHE_TIME,
+                maxAge: millisecondsInMinute,


I don't think the named constant adds any value, and I haven't seen this "pattern" anywhere else in the code, so I just inlined it since there's no need for the // 60s comment anymore.

I would prefer minutesToMilliseconds(1)

I agree! I'll switch to that everywhere so it's consistent.

theneva · 2021-10-27T13:47:17Z

src/test/e2e/api/admin/metrics.e2e.test.ts

    await db.stores.clientInstanceStore.insert({
        appName: 'demo-app-1',
        instanceId: 'test-1',
        strategies: ['default'],
-        started: 1516026938494,
+        started: clientStartedDate,


$ node > new Date(1516026938494) 2018-01-15T14:35:38.494Z > require('date-fns').parseISO('2018-01-15T14:35:38.494Z'); 2018-01-15T14:35:38.494Z

I think it's nice to not have gigantic numbers just hanging out in the code, so I prefer it like this.

theneva · 2021-10-27T13:48:47Z

src/lib/db/session-store.ts

@@ -72,7 +73,7 @@ export default class SessionStore implements ISessionStore {
            .insert({
                sid: data.sid,
                sess: JSON.stringify(data.sess),
-                expired: data.expired || new Date(Date.now() + 86400000),
+                expired: data.expired || addDays(Date.now(), 1),


$ node > require('date-fns').hoursToMilliseconds(24) 86400000

theneva · 2021-10-27T13:49:22Z

src/test/e2e/services/user-service.e2e.test.ts

-                originalMaxAge: 2880000,
-                expires: new Date(Date.now() + 86400000).toDateString(),
+                originalMaxAge: minutesToMilliseconds(48),
+                expires: addDays(Date.now(), 1).toDateString(),


$ node > require('date-fns').hoursToMilliseconds(24) 86400000

theneva · 2021-10-27T13:50:50Z

src/test/e2e/services/user-service.e2e.test.ts

@@ -141,8 +142,8 @@ test("deleting a user should delete the user's sessions", async () => {
        sid: 'xyz321',
        sess: {
            cookie: {
-                originalMaxAge: 2880000,
-                expires: new Date(Date.now() + 86400000).toDateString(),
+                originalMaxAge: minutesToMilliseconds(48),


$ node > require('date-fns').minutesToMilliseconds(48) 2880000

I have no idea why this is 48 minutes, but it is, so I kept it. Let me know if I missed some kind of significance about this number 🤷

I can't see any good reason other than a small mistake when the test was written. I would assume 48 hours make more sense, as this is our default session length.

Maybe @chriswk remembers this part?

I guess the code could just be confused about seconds and milliseconds. Even though the Max-Age and Expiry parts of a set-cookie header use seconds, express-session uses milliseconds (for compatibility with the JS Date, I guess). Multiplying the number by 1000 doesn't get us to 48 hours though 😅 I'm happy to change it to 48 hours if you prefer that – it is just a test, after all.

I do not think that's needed.

theneva · 2021-10-27T13:51:27Z

src/test/e2e/stores/client-metrics-store-v2.e2e.test.ts

-    d1.setHours(10, 10, 11);
-    d2.setHours(11, 10, 11);
+    const d1 = set(Date.now(), { hours: 10, minutes: 10, seconds: 11 });
+    const d2 = addHours(d1, 1);


I think the spirit of d2 is to be d1 + 1 hour, so I changed it a bit. The values are the same, though

theneva · 2021-10-27T14:04:10Z

src/lib/services/client-metrics/ttl-list.js

        this.startTimer();
    }

    destroy() {
-        // https://github.com/nodejs/node/issues/9561
-        // clearTimeout(this.timer);
-        // this.timer = null;


I believe nodejs/node#9561 was fixed in 2016 and was patched into both Node 4 and 6, so this code should be safe to run 😄

ttl-list will be removed in Unleash v4.3.

alright, should I just remove this particular change? I still need some diff in the file to avoid depending on moment.

no, good to remove the moment dependency. Feel free to fix the destroy to, its good to not leave timers hanging around if we do not have too.

okay, I'll leave my original change in, then 😄

theneva · 2021-10-27T14:05:46Z

src/lib/services/client-metrics/client-metrics.test.ts

+    subSeconds,
+} from 'date-fns';
+
+/**


I went a little bit wild explaining this, because it took me a while to figure out what was happening. Let me know if you don't want this long explanation, or if this utility belongs somewhere else in the project!

Thanks for explaining this, I learned something new, so good to keep it here!

We might reuse it later, but let's keep it here for now.

theneva · 2021-10-28T08:46:57Z

src/lib/services/client-metrics/ttl-list.js

@@ -36,26 +36,22 @@ module.exports = class TTLList extends EventEmitter {
    }

    add(value, timestamp = new Date()) {
-        const ttl = moment(timestamp).add(this.expireAmount, this.expireType);
-        if (moment().isBefore(ttl)) {
+        const ttl = add(timestamp, { [this.expireType]: this.expireAmount });


I found a small issue with this: it doesn't support milliseconds. It shouldn't affect the production code (which only uses hours and minutes), but it does affect the tests. I have no idea why they pass… Pushing a fix in a sec

fixed in 442c0b9. maybe it would be better to have the list not support milliseconds? I wanted to keep the the functional changes to a minimum

ivarconr

Looks really good. just a few minor comments

src/lib/addons/addon.test.ts

src/lib/create-config.ts

src/lib/db/client-metrics-db.ts

src/lib/metrics.ts

ivarconr · 2021-10-28T11:55:36Z

src/lib/services/addon-service.ts

@@ -75,7 +74,7 @@ export default class AddonService {
            async () => addonStore.getAll({ enabled: true }),
            {
                promise: true,
-                maxAge: ADDONS_CACHE_TIME,
+                maxAge: millisecondsInMinute,


I would prefer minutesToMilliseconds(1)

src/lib/services/api-token-service.ts

ivarconr · 2021-10-28T11:58:21Z

src/lib/services/client-metrics/client-metrics.test.ts

+    subSeconds,
+} from 'date-fns';
+
+/**


Thanks for explaining this, I learned something new, so good to keep it here!

We might reuse it later, but let's keep it here for now.

ivarconr · 2021-10-28T12:00:42Z

src/lib/services/client-metrics/ttl-list.js

        this.startTimer();
    }

    destroy() {
-        // https://github.com/nodejs/node/issues/9561
-        // clearTimeout(this.timer);
-        // this.timer = null;


no, good to remove the moment dependency. Feel free to fix the destroy to, its good to not leave timers hanging around if we do not have too.

ivarconr · 2021-10-28T12:02:00Z

src/test/e2e/api/admin/metrics.e2e.test.ts

    await db.stores.clientInstanceStore.insert({
        appName: 'demo-app-1',
        instanceId: 'test-1',
        strategies: ['default'],
-        started: 1516026938494,
+        started: clientStartedDate,


ivarconr · 2021-10-28T12:06:08Z

src/test/e2e/services/user-service.e2e.test.ts

@@ -141,8 +142,8 @@ test("deleting a user should delete the user's sessions", async () => {
        sid: 'xyz321',
        sess: {
            cookie: {
-                originalMaxAge: 2880000,
-                expires: new Date(Date.now() + 86400000).toDateString(),
+                originalMaxAge: minutesToMilliseconds(48),


I can't see any good reason other than a small mistake when the test was written. I would assume 48 hours make more sense, as this is our default session length.

Maybe @chriswk remembers this part?

…econds in tests

theneva · 2021-11-02T11:41:29Z

src/lib/services/client-metrics/client-metrics.test.ts

@@ -252,11 +295,11 @@ test('should have correct values for lastMinute', () => {
    let c = metrics.getTogglesMetrics();
    expect(c.lastMinute.toggle).toEqual({ yes: 20, no: 20 });

-    jest.advanceTimersByTime(10 * 1000);
+    jest.advanceTimersByTime(10_000);


yep, having played with it a bit, I think anything below 1 minute is better written as a constant with underscores than either 10 * 1000 or secondsToMilliseconds(10) in the case of jest.advanceTimersByTime. When reading the test, you already know that this function takes milliseconds, so it's better to avoid the noise of helper functions or maths

theneva · 2021-11-02T11:42:31Z

src/lib/services/client-metrics/client-metrics.test.ts

    c = metrics.getTogglesMetrics();
    expect(c.lastHour.toggle).toEqual({ yes: 41, no: 41 });

    // at 30
-    jest.advanceTimersByTime(30 * 60 * 1000);
+    jest.advanceTimersByTime(minutesToMilliseconds(30));


… but when we're dealing with minutes, the numbers are less easy to grok, so I prefer the helper functions over either a constant number of milliseconds or inline arithmetic

theneva · 2021-11-02T11:45:23Z

src/lib/services/client-metrics/ttl-list.test.js


    list.destroy();
    jest.useRealTimers();
 });
+
+test('should add item created in the past but expiring in the future', () => {


I added this to ensure it both works with milliseconds and to catch a bug that I almost introduced and the other tests didn't catch

theneva · 2021-11-02T11:47:12Z

Okay, I think this is ready for a final look 😄

ivarconr

LGTM!

feat: be explicit when specifying time & replace moment with date-fns

1278448

theneva commented Oct 27, 2021

View reviewed changes

chore: remove unused variable. oops!

dc8397c

theneva commented Oct 27, 2021

View reviewed changes

theneva commented Oct 28, 2021

View reviewed changes

fix: make ttl-list support milliseconds again

442c0b9

ivarconr self-requested a review October 28, 2021 11:44

ivarconr reviewed Oct 28, 2021

View reviewed changes

theneva added 2 commits November 2, 2021 12:25

use only date-fns functions, and avoid them for advancing timers by s…

4ec850d

…econds in tests

Merge remote-tracking branch 'origin/master' into explicit-times

cf6d74e

theneva commented Nov 2, 2021

View reviewed changes

ivarconr approved these changes Nov 2, 2021

View reviewed changes

ivarconr merged commit b47e228 into Unleash:master Nov 2, 2021

theneva deleted the explicit-times branch November 2, 2021 14:46

feat: be explicit when specifying time & replace moment with date-fns #1072

feat: be explicit when specifying time & replace moment with date-fns #1072

Conversation

theneva commented Oct 27, 2021

theneva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivarconr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theneva Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theneva commented Nov 2, 2021

ivarconr left a comment

Choose a reason for hiding this comment

theneva Nov 2, 2021 •

edited

Loading