feat: Support encoding and decoding of non-moment date objects #78

jimmycallin · 2023-02-15T22:37:15Z

Resolves FT-bc5b3fba-b1ca-11ed-ba1e-a2da51ff8a88

I have added automatic tests where applicable
The PR title is suitable as a release note
The PR contains a description of what has been changed
The description contains manual test instructions

Changes

As part of easing migration from moment, as described in #71 , we start by supporting encoding of non-moment date objects. This includes all date libraries that have toISOString as a method on their date objects, as well as native Date library, and also an ISO6801 compatible string.

Test

Make sure encoding all these date formats work as expected.

lucaas

Did a first pass on the code

lucaas · 2023-02-16T10:27:37Z

source/session.ts

@@ -307,7 +306,8 @@ export class Session {
      return out;
    }

-    if (data && data._isAMomentObject) {
+    const date = convertToISOString(data);


Should we try to convert all strings and numbers, or should we separate checking for date-like objects and encoding to iso string?

Not sure what you mean by this comment. currently we support converting all date objects, including moment and dayjs that have toISOString method - as well as converting ISO 8601 compatible strings to UTC. Do you want to split strings and number cases from object cases?

Basically, the question was if we should do:

if (isDateLike(data)) { data = convertToISOString(data) }

Rather than:

date = convertToISOString(data) if (date) { data = date; }

I am not convinced that we should. I just thought it looked odd to try converting all types (except arrays and pojo:s already handled).

I don't mind either way, so let me know if you want me to change it

lucaas · 2023-02-16T10:30:19Z

source/util/convert_to_iso_string.ts

@@ -0,0 +1,20 @@
+function isIsoDate(str: string) {
+  if (!/\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z/.test(str)) return false;


Should we also encode strings in a non-UTC timezone?

we're encoding strings in a non-utc timezone, e.g. 2023-01-01T01:00:00+01:00, and converting to UTC. see tests.

The previous regex wasn't matching them - now it looks better.

lucaas · 2023-02-16T10:31:17Z

source/util/convert_to_iso_string.ts

+    // if this is a date object of type moment or dayjs, or regular date object (all of them has toISOString)
+    ((typeof data !== "string" && typeof data.toISOString === "function") ||
+      // if it's a ISO string already
+      (typeof data == "string" && isIsoDate(data)))


Why do we encode already formatted strings?

we support non-UTC timezones, while the server doesn't. this is to make sure they are in UTC format, since toISOString always return UTC format.

Just checking, do we have a use case to supporting these? It seems like you have to jump through a few hoops to format dates like this.

Nothing that I'm aware of, I thought it was better to support encoding all ISO8601 compatible strings than just a subset of them.

source/session.ts

lucaas · 2023-02-16T10:39:38Z

source/session.ts

        };
      }

      // Ensure that the moment object is in local time zone and format
      // to timezone naive string.
      return {
        __type__: "datetime",
-        value: data.local().format(ENCODE_DATETIME_FORMAT),
+        value: moment(date).local().format(ENCODE_DATETIME_FORMAT),


Should we target the v2 for these changes as well? I am not really sure how to test this for regressions, so feels safer to do this while also removing this branch.

i have intentionally separated code for this PR to be backwards compatible by making sure all moment objects are passed in the same manner as before, so I hope we can target this for a minor release. we can add additional tests for this if there are certain use cases you are unsure about.

I want to ensure that this path hasn't changed, without having to disable timezone support locally.

Can we add a test for:

const date = moment() const convertedDate = convertToISOString(date) const encodedDate = moment(date).local().format(ENCODE_DATETIME_FORMAT) assert date.local().format(ENCODE_DATETIME_FORMAT) === encodedDate

please see new tests!

lucaas · 2023-02-16T10:42:26Z

source/session.ts

-        // need to strip from query.
-        value = moment(value).utc().format(ENCODE_DATETIME_FORMAT);
+      } else if (convertToISOString(value)) {
+        value = convertToISOString(value);


Is the comment not true?

// Server does not store microsecond or timezone currently so
// need to strip from query.

These will be translated to a SQL comparison, so I think we need to strip to match how the dates are stored in the database.

it's true that the server currently does not store microseconds, but I believe it's better to allow sending in all ISO8691 validated strings and convert them to whatever is supported in the server (as it currently works), than removing microseconds in the client.

server does store timezones in a way by only supporting UTC timezone, and we are thus converting to UTC dates always.

I tried testing this manually, and it does look like it works, though I am not sure why.. both of these return the expected Note.

select id from Note where thread_activity is "2023-02-20 09:58:01" select id from Note where thread_activity is "2023-02-20T09:58:01.102Z"

Having a date-times as identifyingKey doesn't make much sense, but I assume we should continue to support it. As long as it does, I am happy.

the server encode all dates to a precision the database handles, which means that it strips the milliseconds (and timezone) from the string, which is why it is backwards compatible.

if the server API one day start supporting milliseconds granularity, this query will stop working yes, but that would be a breaking change from the server API anyway.

source/util/convert_to_iso_string.ts

lucaas · 2023-02-20T10:05:50Z

source/session.ts

-        // need to strip from query.
-        value = moment(value).utc().format(ENCODE_DATETIME_FORMAT);
+      } else if (convertToISOString(value)) {
+        value = convertToISOString(value);


I tried testing this manually, and it does look like it works, though I am not sure why.. both of these return the expected Note.

select id from Note where thread_activity is "2023-02-20 09:58:01" select id from Note where thread_activity is "2023-02-20T09:58:01.102Z"

Having a date-times as identifyingKey doesn't make much sense, but I assume we should continue to support it. As long as it does, I am happy.

lucaas · 2023-02-20T10:30:20Z

source/session.ts

@@ -307,7 +306,8 @@ export class Session {
      return out;
    }

-    if (data && data._isAMomentObject) {
+    const date = convertToISOString(data);


Basically, the question was if we should do:

if (isDateLike(data)) { data = convertToISOString(data) }

Rather than:

date = convertToISOString(data) if (date) { data = date; }

I am not convinced that we should. I just thought it looked odd to try converting all types (except arrays and pojo:s already handled).

lucaas · 2023-02-20T10:38:59Z

source/session.ts

        };
      }

      // Ensure that the moment object is in local time zone and format
      // to timezone naive string.
      return {
        __type__: "datetime",
-        value: data.local().format(ENCODE_DATETIME_FORMAT),
+        value: moment(date).local().format(ENCODE_DATETIME_FORMAT),


I want to ensure that this path hasn't changed, without having to disable timezone support locally.

Can we add a test for:

const date = moment() const convertedDate = convertToISOString(date) const encodedDate = moment(date).local().format(ENCODE_DATETIME_FORMAT) assert date.local().format(ENCODE_DATETIME_FORMAT) === encodedDate

lucaas

Looks good and working as intended. Consider if we should rename ISO to Iso for consistency.

lucaas · 2023-02-22T10:18:36Z

source/session.ts

@@ -96,18 +96,20 @@ export interface ResponseError {
  error?: Data;
 }

-export interface MutatationOptions {
+export interface MutationOptions {


lucaas · 2023-02-22T10:21:25Z

source/session.ts

+  private decode(
+    data: any,
+    identityMap: Data = {},
+    decodeDatesAsISO: boolean = false


I was bout to comment that we could make the third argument a decodeOptions to future-proof if we have a need for more options, but seeing as this is assumed to be private - I don't think it matters.

source/session.ts

* feat: support encoding non-moment date objects * add tests for convert_to_iso_string * added docstring * add additional encoding tests * support non-moment decoding * non-UTC timezone for tests in CI * consistent camel casing

jimmycallin requested a review from a team as a code owner February 15, 2023 22:37

jimmycallin force-pushed the support-non-moment-encoding branch 4 times, most recently from 706b3ac to 69981a7 Compare February 15, 2023 22:45

jimmycallin requested review from gismya, lucaas and mlagergren February 15, 2023 22:45

lucaas requested changes Feb 16, 2023

View reviewed changes

jimmycallin requested a review from lucaas February 19, 2023 23:08

lucaas requested changes Feb 20, 2023

View reviewed changes

jimmycallin added 4 commits February 20, 2023 12:02

feat: support encoding non-moment date objects

163d34e

add tests for convert_to_iso_string

99c1c8e

added docstring

4604cdc

add additional encoding tests

e401e39

jimmycallin force-pushed the support-non-moment-encoding branch from 069e987 to e401e39 Compare February 20, 2023 11:11

jimmycallin requested a review from lucaas February 20, 2023 11:23

support non-moment decoding

001eb83

jimmycallin changed the title ~~feat: support encoding non-moment date objects~~ feat: support encoding and decoding of non-moment date objects Feb 20, 2023

non-UTC timezone for tests in CI

26351d4

jimmycallin changed the title ~~feat: support encoding and decoding of non-moment date objects~~ feat: Support encoding and decoding of non-moment date objects Feb 21, 2023

Merge branch 'main' into support-non-moment-encoding

83ed053

lucaas reviewed Feb 22, 2023

View reviewed changes

lucaas approved these changes Feb 22, 2023

View reviewed changes

consistent camel casing

0394d8b

jimmycallin merged commit bc82086 into main Feb 22, 2023

jimmycallin deleted the support-non-moment-encoding branch February 22, 2023 10:55

		@@ -0,0 +1,20 @@
		function isIsoDate(str: string) {
		if (!/\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z/.test(str)) return false;

feat: Support encoding and decoding of non-moment date objects #78

feat: Support encoding and decoding of non-moment date objects #78

Uh oh!

Conversation

jimmycallin commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Test

Uh oh!

lucaas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimmycallin Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucaas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jimmycallin commented Feb 15, 2023 •

edited

Loading

jimmycallin Feb 16, 2023 •

edited

Loading