Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix absolute-date #32375

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Fix absolute-date #32375

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e094f54
fix date
wxiaoguang Oct 30, 2024
c9d7263
Merge branch 'main' into fix-date
GiteaBot Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 0 additions & 16 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
"sortablejs": "1.15.2",
"swagger-ui-dist": "5.17.14",
"tailwindcss": "3.4.10",
"temporal-polyfill": "0.2.5",
"throttle-debounce": "5.0.2",
"tinycolor2": "1.6.0",
"tippy.js": "6.3.7",
Expand Down
8 changes: 7 additions & 1 deletion web_src/js/webcomponents/absolute-date.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,15 @@ test('toAbsoluteLocaleDate', () => {
day: 'numeric',
})).toEqual('March 15, 2024');

expect(toAbsoluteLocaleDate('2024-03-15', 'de-DE', {
expect(toAbsoluteLocaleDate('2024-03-15T01:02:03', 'de-DE', {
year: 'numeric',
month: 'long',
day: 'numeric',
})).toEqual('15. März 2024');
Copy link
Member

@silverwind silverwind Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the output of the function for inputs 2024-03-15, de-DE now?

There are cases where only date without time is passed to toAbsoluteLocaleDate, for example for milestones. I think this change may break those cases depending on the timezone in which it is ran.

I think Temporal.PlainDate is the only JS API capable of dealing correctly with date-only values (Date can not as it deals with timestamps that will shift with time zone).

Copy link
Contributor Author

@wxiaoguang wxiaoguang Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Contributor Author

@wxiaoguang wxiaoguang Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

image

image

Copy link
Member

@silverwind silverwind Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is:

> new Date("2024-03-15")
Date Fri Mar 15 2024 01:00:00 GMT+0100 (Central European Standard Time)

This date object has a time component which is wrong because dates are timeless. Is it guaranteed that the output of the following is always the same, regardless of system time and timezone?

> new Date("2024-03-15").toLocaleString("de-DE", {year: "numeric", month: "long", day: "numeric"})
15. März 2024

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not related.

Using a backend date string is wrong, for long time. That string is in server's timezone, is a string, doesn't have the timezone field, and frontend don't know, it would definitely go wrong when the browser's timezone is different from the server's.

To make the "deadline" right, it needs to completely refactor the backend first.

ps: the frontend use the string "date" part is also wrong, but I didn't change that logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we only need to do new Date(date + 'T00:00:00'), then the result is right.

Copy link
Member

@silverwind silverwind Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even easier reproduction, this should be possible to simulate in unit tests:

> process.env.TZ = "US/Eastern"; new Date("2024-03-15").toLocaleString("de-DE", {year: "numeric", month: "long", day: "numeric"})
'14. März 2024'
> process.env.TZ = "Europe/Berlin"; new Date("2024-03-15").toLocaleString("de-DE", {year: "numeric", month: "long", day: "numeric"})
'15. März 2024'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we only need to do new Date(date + 'T00:00:00'), then the result is right.

Yes, but why abuse an API that was not made for this use case? Temporal.PlainDate was made for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-> Fix toAbsoluteLocaleDate and add more tests #32387

Added more tests.

Temporal.PlainDate was made for this.

It just introduces more unnecessary polyfills, make the code unnecessary larger, and according to MDN: It is not yet ready for production use! (just quoted the sentence).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not yet ready for production use

Right, it's still in stage 3, so not finalized and will probably take another year or two until we see it being implemented in browsers.


expect(toAbsoluteLocaleDate('12345-03-15 01:02:03', '', {
year: 'numeric',
month: 'short',
day: 'numeric',
})).toEqual('Mar 15, 12345');
});
30 changes: 14 additions & 16 deletions web_src/js/webcomponents/absolute-date.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,28 @@
import {Temporal} from 'temporal-polyfill';

export function toAbsoluteLocaleDate(dateStr, lang, opts) {
return Temporal.PlainDate.from(dateStr).toLocaleString(lang ?? [], opts);
export function toAbsoluteLocaleDate(date: string, lang: string, opts: Intl.DateTimeFormatOptions) {
return new Date(date).toLocaleString(lang || [], opts);
}

window.customElements.define('absolute-date', class extends HTMLElement {
static observedAttributes = ['date', 'year', 'month', 'weekday', 'day'];

initialized = false;

update = () => {
const year = this.getAttribute('year') ?? '';
const month = this.getAttribute('month') ?? '';
const weekday = this.getAttribute('weekday') ?? '';
const day = this.getAttribute('day') ?? '';
const opt: Intl.DateTimeFormatOptions = {};
for (const attr of ['year', 'month', 'weekday', 'day']) {
if (this.getAttribute(attr)) opt[attr] = this.getAttribute(attr);
}
const lang = this.closest('[lang]')?.getAttribute('lang') ||
this.ownerDocument.documentElement.getAttribute('lang') || '';

// only use the first 10 characters, e.g. the `yyyy-mm-dd` part
const dateStr = this.getAttribute('date').substring(0, 10);
// only use the date part, it is guaranteed to be in ISO format (YYYY-MM-DDTHH:mm:ss.sssZ)
let date = this.getAttribute('date');
let dateSep = date.indexOf('T');
dateSep = dateSep === -1 ? date.indexOf(' ') : dateSep;
date = dateSep === -1 ? date : date.substring(0, dateSep);

if (!this.shadowRoot) this.attachShadow({mode: 'open'});
this.shadowRoot.textContent = toAbsoluteLocaleDate(dateStr, lang, {
...(year && {year}),
...(month && {month}),
...(weekday && {weekday}),
...(day && {day}),
});
this.shadowRoot.textContent = toAbsoluteLocaleDate(date, lang, opt);
};

attributeChangedCallback(_name, oldValue, newValue) {
Expand Down