to-offset-date-time #27087

qiaozha · 2023-09-12T04:06:19Z

Packages impacted by this PR

@azure/core-util

Issues associated with this PR

Describe the problem that is addressed by this PR

In TypeSpec, we have a offsetDateTime type, which is suppose to provide the timezone difference when the format is rfc3339, like
2023-09-11T18:45:20.988+08:00

but typescript date.toISOString() doesn't have such info. as there's no easy way to convert it, I think it might be appropriate to add the convert function into core-util.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR:***(Applicable only to SDK release request PRs)*

Checklists

Added impacted package name to the issue description
Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
Added a changelog (if necessary)

qiaozha · 2023-09-12T04:07:41Z

sdk/core/core-util/test/public/datetime.spec.ts

+  describe("work in +08:00 timezone", function () {
+    it("converts a base64 string to bytes", function () {
+      const date = new Date("2022-01-05T19:08:10Z");
+      assert.strictEqual(toOffsetDateTime(date), "2022-01-05T19:08:10.000+08:00");


@xirzec I don't think the test case is correct for all timezones, but have no idea how to set a specific timezone, so that it works

azure-sdk · 2023-09-12T04:22:08Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-core-util

qiaozha · 2023-09-12T06:09:04Z

sdk/core/core-util/test/public/datetime.spec.ts

+      const date = new Date("2022-01-05T19:08:10.000Z");
+      assert.strictEqual(
+        Date.parse(toOffsetDateTime(date)) - Date.parse(date.toISOString()),
+        60 * date.getTimezoneOffset() * 1000


A little unsure whether the difference should be 0 ?
if it's 2022-01-05T19:08:10.000Z In UTC time, then it should be 2022-01-06T03:08:10.000+08:00 in the +08:00 timezone. so maybe purely use something like the below in toOffsetDateTime is incorrect?
date.toISOString().replace(/Z$/, "") + diff + pad(tzOffset / 60) + ":" + pad(tzOffset % 60)

I think the difference should be 0 here, no matter the ISO datetime or offset datetime they are just format differences and the underlying number should be the one.

Not sure it's easy to convert them. If not, we may take the offsetDateTime as utcDatetime in typespec.

I notice that we didn't import any library to handle date and time. Not sure this is mainly for performance consideration.

I know with momentjs we could easily format any date (docs).

> moment.defaultFormat 'YYYY-MM-DDTHH:mm:ssZ' > moment.defaultFormatUtc 'YYYY-MM-DDTHH:mm:ss[Z]'

My understanding is that moment is an extremely heavy dependency and something that will eventually be replaced by an ecmascript standard /cc @bterlson

MaryGao · 2023-09-12T10:27:15Z

sdk/core/core-util/test/public/datetime.spec.ts

+import { toOffsetDateTime } from "../../src/datetime";
+import { assert } from "chai";
+
+describe("utcDateTime with timezone offset", function () {


Could we add more cases to cover corner situations e.g +0, -11?

As stated here #27087 (comment)

Let me know if you can figure out a way.

…rce of truth

qiaozha

do some self reviews.

qiaozha · 2023-09-14T12:37:44Z

sdk/core/core-util/test/public/datetime.spec.ts

+    });
+    it("the to offset datetime should be correct in 0 milliseconds precision", function () {
+      const date = new Date();
+      assert.strictEqual(toOffsetDateTime(date, 0), moment(date).format("YYYY-MM-DDTHH:mm:ssZ"));


As I am not sure how to set the timezone, I realize we can think of the result of moment format as the source of truth to test the converter helper. if this tests can pass, then at least, the converter should be able to work in my local timezone and the one that the CI runs.

qiaozha · 2023-09-14T12:38:41Z

sdk/core/core-util/test/public/datetime.spec.ts

+    });
+  });
+
+  describe("work in 2022-01-05T19:08:10.000Z", function () {


add this one because of the milliseconds is 0, and we might miss to pad 0s to the end.

qiaozha · 2023-09-14T12:39:43Z

sdk/core/core-util/src/datetime.ts

+// Licensed under the MIT license.
+export type OffsetMillisecondsPrecision = 0 | 1 | 2 | 3;
+
+export function toOffsetDateTime(date: Date, precision: OffsetMillisecondsPrecision = 0): string {


add an option as the precision of milliseconds we want to keep. by default it's 0 now, but let me know if there;s other opppinion.

qiaozha · 2023-09-14T12:40:18Z

sdk/core/core-util/src/datetime.ts

+    pad(date.getMinutes()) +
+    ":" +
+    pad(date.getSeconds()) +
+    (precision !== 0 ? `.${precisedMilliseconds}`.padEnd(precision + 1, "0") : "") +


padEnd first parameter is precison + 1 because there's a dot in the string.

qiaozha · 2023-09-14T12:47:43Z

sdk/core/core-util/test/public/datetime.spec.ts

+      const date = new Date();
+      assert.strictEqual(
+        Math.floor(
+          Math.abs(Date.parse(toOffsetDateTime(date)) - Date.parse(date.toISOString())) / 1000


Here as the toOffsetDateTime will lost the miliiseconds of three digit, and we have no way to assert the real diff, so I think we can assume the quotient of the diff divided by 1000 is zero.

qiaozha · 2023-09-14T12:48:50Z

sdk/core/core-util/test/public/datetime.spec.ts

+      const date = new Date("2022-01-05T19:08:10.000Z");
+      // assert.strictEqual(moment(date).format(), "2022-01-06T03:08:10+08:00");
+      assert.strictEqual(toOffsetDateTime(date), moment(date).format());
+      assert.strictEqual(Date.parse(toOffsetDateTime(date)) - Date.parse(date.toISOString()), 0);


As the milliseconds is zero, we don't have unixtime diff loss. so the diff should be zero for all precisions.

qiaozha · 2023-09-14T12:57:46Z

sdk/core/core-util/package.json

@@ -105,7 +105,8 @@
    "sinon": "^15.0.0",
    "typescript": "~5.0.0",
    "util": "^0.12.1",
-    "ts-node": "^10.0.0"
+    "ts-node": "^10.0.0",
+    "moment": "^2.29.4"


this is the dev dependency, so I guess it's okay ?

MaryGao · 2023-09-14T14:09:05Z

sdk/core/core-util/src/datetime.ts

+  const pad = (n: number) => `${Math.floor(Math.abs(n))}`.padStart(2, "0");
+  const precisedMilliseconds = getPrecisedMilliseconds(date, precision);
+  return (
+    date.getFullYear() +


I was wondering if this implementation would work if timezone is crossed year for example utc is in 2023, but with offset timezone it may 2022. The same question for crossed month and day.

Just realize these interfaces use the local timezone
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/getFullYear

xirzec · 2023-09-14T18:42:23Z

Found this for mocking the timezone in Node-only tests at least: https://www.npmjs.com/package/timezone-mock

To step back for a bit, I'm not sure I understand what this datatype means from a TypeSpec perspective, especially for roundtripping. Like when a client retrieves a value from the service, is the offset based on the service's timezone? Or based on the last client that updated that field? If the timezone in the local client is different, do we change the value when doing a PUT even if the developer didn't touch that field?

This seems like an extremely error-prone feature of the language. @bterlson can you explain why TypeSpec has this type?

xirzec · 2023-09-14T19:16:48Z

Talked to Brian offline, for now we can be lossy here - clients won't know the original offset that came from the service (we convert to Date) and we'll always serialize back to the local TZ. Eventually with Temporal we can do better, but not holding my breath for runtime support.

xirzec · 2023-09-14T19:26:50Z

Second update: After more discussion @bterlson 's preference is to leave this type as string and not serialize/deserialize it to Date. Can we change the codegen to do that instead @qiaozha ?

qiaozha · 2023-09-14T23:30:27Z

@xirzec yeah, we can totally do that, and it makes much more sense to me. Because I also thougtht of that when client side get the response with a date time which is a offsetDateTime, customers will probably have to convert it by themselves, then I thought maybe they could leverage this helper function or other util whatever they like. Should I just close this PR ?

but I want to confirm with @bterlson, do you think we should make this cross language consistent ?

qiaozha · 2023-09-15T00:47:22Z

close it as offline confirmed with Brian, the current behavior of other language treating offsetDateTime exactly the same as utcDateTime is incorrect, but they can handle this as appropriate.

to-offset-date-time

4c31be5

qiaozha requested review from bterlson, xirzec, jeremymeng and deyaaeldeen as code owners September 12, 2023 04:06

github-actions bot added the Azure.Core label Sep 12, 2023

qiaozha commented Sep 12, 2023

View reviewed changes

fix format

5423738

try fix the test cases

48c658b

qiaozha commented Sep 12, 2023

View reviewed changes

MaryGao reviewed Sep 12, 2023

View reviewed changes

qiaozha added 3 commits September 13, 2023 08:25

not using toISOString replace

4a972a9

remove comment code

97cc99b

add millisecond precision option and add tests to treat moment as sou…

dbcd6c6

…rce of truth

qiaozha requested a review from ckairen as a code owner September 14, 2023 12:33

qiaozha commented Sep 14, 2023

View reviewed changes

merge main

55722df

qiaozha commented Sep 14, 2023

View reviewed changes

MaryGao reviewed Sep 14, 2023

View reviewed changes

qiaozha closed this Sep 15, 2023

qiaozha mentioned this pull request Sep 15, 2023

use-string-as-offset-datetime-type-in-modular Azure/autorest.typescript#2019

Merged

qiaozha mentioned this pull request Sep 22, 2023

fix-offset-datetime-in-rlc Azure/autorest.typescript#2031

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

to-offset-date-time #27087

to-offset-date-time #27087

qiaozha commented Sep 12, 2023 •

edited

Loading

qiaozha Sep 12, 2023

azure-sdk commented Sep 12, 2023

qiaozha Sep 12, 2023

MaryGao Sep 12, 2023 •

edited

Loading

xirzec Sep 12, 2023

MaryGao Sep 12, 2023 •

edited

Loading

qiaozha Sep 13, 2023

qiaozha Sep 13, 2023

qiaozha left a comment

qiaozha Sep 14, 2023

qiaozha Sep 14, 2023

qiaozha Sep 14, 2023

qiaozha Sep 14, 2023 •

edited

Loading

qiaozha Sep 14, 2023

qiaozha Sep 14, 2023

qiaozha Sep 14, 2023

MaryGao Sep 14, 2023

MaryGao Sep 14, 2023

xirzec commented Sep 14, 2023

xirzec commented Sep 14, 2023

xirzec commented Sep 14, 2023

qiaozha commented Sep 14, 2023

qiaozha commented Sep 15, 2023 •

edited

Loading

to-offset-date-time #27087

to-offset-date-time #27087

Conversation

qiaozha commented Sep 12, 2023 • edited Loading

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Are there test cases added in this PR? (If not, why?)

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

Choose a reason for hiding this comment

azure-sdk commented Sep 12, 2023

Choose a reason for hiding this comment

MaryGao Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaryGao Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qiaozha 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

qiaozha Sep 14, 2023 • edited Loading

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

xirzec commented Sep 14, 2023

xirzec commented Sep 14, 2023

xirzec commented Sep 14, 2023

qiaozha commented Sep 14, 2023

qiaozha commented Sep 15, 2023 • edited Loading

qiaozha commented Sep 12, 2023 •

edited

Loading

Command used to generate this PR:***(Applicable only to SDK release request PRs)*

MaryGao Sep 12, 2023 •

edited

Loading

MaryGao Sep 12, 2023 •

edited

Loading

qiaozha Sep 14, 2023 •

edited

Loading

qiaozha commented Sep 15, 2023 •

edited

Loading