fix(dates): improve date string parsing #1646
Conversation
Use Luxon to parse date/datetime strings. This avoids the `Date.parse`'s inconsistency between date-only (assumed UTC) and datetime (assumed local timezone) strings. (closes jackyzha0#1615) It also allows the date string's timezone to be carried along with the DateTime object, producing more friendly and semantically-correct timestamps.
|
Hey, thanks for this PR. I have a few questions. I'll leave a detailed review of the code changes later today.
|
Dates are hidden in the content meta if the file does not have them. This has not changed. In both the main branch ( Prior to this PR,
The actual rendering of visible dates in the pages is done in -export function formatDate(d: Date, locale: ValidLocale = "en-US"): string {
- return d.toLocaleDateString(locale, {
- year: "numeric",
- month: "short",
- day: "2-digit",
- })
+export function formatDate(d: DateTime, locale: ValidLocale = "en-US"): string {
+ return d.toLocaleString(
+ {
+ year: "numeric",
+ month: "short",
+ day: "2-digit",
+ },
+ { locale: locale },
+ )
}
export function Date({ date, locale }: Props) {
- return <time datetime={date.toISOString()}>{formatDate(date, locale)}</time>
+ return <time datetime={date.toISO() || ""}>{formatDate(date, locale)}</time>
}The output of the Luxon's Furthermore, javascript's
The way quartz currently works, the timestamp is hard-coded into the HTML of the page, so the only thing that matters is the timezone of the system at build-time, and the default timezone from the config. given these options in ...
locale: "de-DE",
timezone: "Europe/Berlin",
defaultDateType: "modified",
...
With a system timezone of ...
locale: "de-DE",
timezone: undefined,
defaultDateType: "modified",
...
tl;dr: With Luxon, the timezone from the datetime has the timezone of the date string. If the datetime string does not have a timezone, it is assumed to belong to the configured |
Quartz's default priority is frontmatter, git, filesystem. Filesystem is always present, usually resulting in the deploy date. By default a date will be displayed, even if a frontmatter key is not present. Therefore, by default, we expect a date to be visible.
This is a change in default behavior, as default configuration always has a date (filesystem)
If I understand correctly, the
Thanks for detailed explaination. I'll get back on this during the weekend. |
baodrate
force-pushed
the
date-parsing
branch
2 times, most recently
from
6cf3b40 to
acef63a
Compare
Yes. It's a sensible default. That particular change (re: missing dates) was a very slight adjustment to make the edge cases a little more consistent. It shouldn't affect the regular use case at all
It is a change in the meaning of (e.g.)
This is accurate, but to avoid any ambiguity: unchanged behavior: The changed behavior: The
Hopefully this truth table is more clear than my previous one. This one assumes your local timezone is
|
Make date handling more consistent such that file dates are optional everywhere, i.e. dates are not rendered unless the CreatedModifiedDate plugin sourced the configured date type.
Explicitly define Content Index types to improve type checking. Make rss feed and sitemap use the appropriate date type: published and modified, respectively.
Allows the user to set fallback values for the `defaultDateType` setting Prior to this commit, if the date was not set, it would default to the current time. e.g. if using `defaultDateType == "published"` or if the CreatedModifiedDate plugin's `priority` setting is set without `filesystem`
baodrate
force-pushed
the
date-parsing
branch
from
acef63a to
43190c4
Compare
|
I might be super wrong and ignorant here, but would it makes sense to infer timezone from locale alone by default? User can still customize timezone if needed, my worry is that introducing another timezone field into the config might confuse user. |
| created ||= file.data.frontmatter.date as MaybeDate | ||
| modified ||= file.data.frontmatter.lastmod as MaybeDate | ||
| modified ||= file.data.frontmatter.updated as MaybeDate | ||
| modified ||= file.data.frontmatter["last-modified"] as MaybeDate | ||
| published ||= file.data.frontmatter.publishDate as MaybeDate |
There was a problem hiding this comment.
tbh we should dealias all of the frontmatter stuff on the frontmatter transformers instead of doing it here.
Let me submit a quick PR for this.
| created ||= DateTime.fromMillis(st.birthtimeMs || Math.min(st.ctimeMs, st.mtimeMs)) | ||
| modified ||= DateTime.fromMillis(st.mtimeMs) |
There was a problem hiding this comment.
this would probably add a bit of performance regression in terms of parsing time.
Can you run a quick bench comparing before and after for build time?
Just my two cents, but most users use the default I feel like it might result in the same confusion as at the moment. Perhaps we should consider only using locale if timezone is missing (with a default to Also consider that locales can easily span several timezones. |
fwiw we can derive timezone from "build" machine, or have a default tz set like shown. but yeah the cartesian of all possible solution might be confusing i afraid |
A set of improvements related to the parsing of date/datetime values.
Changes:
Date.parse's inconsistency between date-only (assumed UTC) and datetime (assumed local timezone) strings. (closes Incorrect timezone conversion for 'date' frontmatter #1615)defaultDateTypesettingSome slight refactoring was done along the way to make type annotations more consistent and type-checking more useful, but effort was made to avoid any changes to how quartz behaves or the structure of the files that are emitted (except where otherwise noted)
Apologies for the large PR. I tried to keep it as small as possible but a lot fo things touch the dates and I didn't want to leave the codebase in a regressed state between PRs. (for ex, the multiple change to
defaultDateTypeis to compensate for the change to the handling of missing dates). Each commit should make sense on its own, if that helps for reviewing. But I can split this PR into multiple ones if requested