fix(dates): improve date string parsing #1646
Conversation
|
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
|
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.
There was a problem hiding this comment.
ideally, I think we should make these customizable, rather than making a list of predefined aliases.
All the obsidian plugins I can find that set the created/modified date (front-matter-timestamps, frontmatter-modified-date, update-time-updater, linter) allow customizing the property that stores the timestamp. It is quite likely a user wishes to use a non-default property name.
Collapsing these as aliases in the frontmatter transformer isn't great, IMO, since it's hard-coding these assumptions even further "upstream". For example, I might want to use published for the date notes are published to the vault, but publishDate as the date a referent (e.g. a book/article) was published in real life. And I might want to display this information in the page on quartz, maybe using a new plugin.
There was a problem hiding this comment.
I think we don't have to, at the end of the day quartz is "opinionated", and we should just support the most populate case, which is what we have here (i believe this aligns with all the frontmatter supported by obsidian publish)
regardless the dealias is separate from this anw
There was a problem hiding this comment.
fair enough. I'll wait for that PR (#1664) to get merged and rebase on top of that
| 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?
There was a problem hiding this comment.
will do. but FWIW, the net computation is for the most part unchanged for this codepath. it goes from coerceDate(st.birthtimeMs), which is basically new Date(st.birthtimeMs), to DateTime.fromMillis(st.birthtimeMs...)
There was a problem hiding this comment.
Benchmark results:
- using hyperfine w/
hyperfine -w5 'npx quartz build' - using https://github.com/RobertTalbert/discretecs-vault as the
contentdirectory (withSTART HERE.mdrenamed toindex.md) - using default
quartz.config.tsexcept forPlugin.CreatedModifiedDate({ priority: ["filesystem"] }) - Build machine is a 2021 MacBook Pro w/ M1 Max CPU
For v4 branch (commit ff9e60a)
Benchmark 1: npx quartz build
Time (mean ± σ): 5.006 s ± 0.026 s [User: 7.043 s, System: 1.184 s]
Range (min … max): 4.953 s … 5.039 s 10 runs
For this PR (commit 5e7580f)
Benchmark 1: npx quartz build
Time (mean ± σ): 5.142 s ± 0.069 s [User: 7.132 s, System: 1.204 s]
Range (min … max): 5.024 s … 5.269 s 10 runs
The difference in build time is quite small but seems to be consistent. I assume it's the overhead of Luxon vs native Date
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 |
I agree with @saberzero1 that this is likely not ideal, since it's quite likely that a user's preferred locale does not correlate with their timezone. Nor do I see an obvious way to do this mapping
Yes, this is the current behavior. For non-timezone datetime strings, we assume the "build" machine's local timezone by default. The setting only changes that assumed timezone if set. (Note that this is the behavior even if the
That's fair, I was also concerned that the setting was not obvious. I will remove the setting. My main motivation was to get quartz to a state where I can run it in a CI system (like github actions), where I don't control the local timezone of the system. But, and I need to confirm this, I believe it is possible to change the effect local time for node by setting the (The downside of this approach is that the "timezone" is half of the information needed to know when the notes in the vault were created. So I would have preferred to at least store it in |
baodrate
force-pushed
the
date-parsing
branch
from
43190c4 to
5e7580f
Compare
|
Reverted the commit that added the timezone setting, and rebased on new changes to |
|
fwiw we can do this in by default if users doesn't specify a specific timezone in If users specify it here, then by default it will apply to all note, then for per-note user can also do that (so that the behaviour also aligns with comments) let me know if this makes sense to you. |
baodrate
force-pushed
the
date-parsing
branch
from
5e7580f to
5a37d7c
Compare
That does indeed seem sensible. I will redo the config commit to put the configuration in the CreatedModifiedDate rather than the app-wide config |
baodrate
force-pushed
the
date-parsing
branch
from
729b84c to
687ec8d
Compare
Thanks so much for the continuous work. Let's try to get this merge |
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.
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
687ec8d to
0b7dafd
Compare
| modified ||= file.data.frontmatter.updated as MaybeDate | ||
| modified ||= file.data.frontmatter["last-modified"] as MaybeDate | ||
| published ||= file.data.frontmatter.publishDate as MaybeDate | ||
| created ||= parseDateString(fp, file.data.frontmatter.date, parseOpts) |
There was a problem hiding this comment.
The datetime properties (created/modified/published) will be dealiased in the FrontMatter plugin (per #1664). Since we're codifying that those properties are datetimes, and frontmatter is the only source that uses string values, does it make sense to do the parsing in frontmatter instead?
i.e.
diff --git a/quartz/plugins/transformers/frontmatter.ts b/quartz/plugins/transformers/frontmatter.ts
index d444b00..8b9d361 100644
--- a/quartz/plugins/transformers/frontmatter.ts
+++ b/quartz/plugins/transformers/frontmatter.ts
@@ -74,16 +74,16 @@ export const FrontMatter: QuartzTransformerPlugin<Partial<Options>> = (userOpts)
const socialImage = coalesceAliases(data, ["socialImage", "image", "cover"])
const created = coalesceAliases(data, ["date", "created"])
- if (created) data.created = created
+ if (created) data.created = parseDateString(created, parseOpts)
const modified = coalesceAliases(data, [
"lastmod",
"updated",
"last-modified",
"modified",
])
- if (modified) data.modified = modified
+ if (modified) data.modified = parseDateString(modified, parseOpts)
const published = coalesceAliases(data, ["publishDate", "published", "date"])
- if (published) data.published = published
+ if (published) data.published = parseDateString(published, parseOpts)
if (socialImage) data.socialImage = socialImage
@@ -103,9 +103,9 @@ declare module "vfile" {
} & Partial<{
tags: string[]
aliases: string[]
- modified: string
- created: string
- published: string
+ modified: DateTime
+ created: DateTime
+ published: DateTime
description: string
publish: boolean | string
draft: boolean | stringThere was a problem hiding this comment.
Yeah I think this makes sense. Actually you can cherrypick that PR into here (I can close that one)
There was a problem hiding this comment.
the other PR has been merged. Feel free to continue the work here.
| modified ||= file.data.frontmatter.updated as MaybeDate | ||
| modified ||= file.data.frontmatter["last-modified"] as MaybeDate | ||
| published ||= file.data.frontmatter.publishDate as MaybeDate | ||
| created ||= parseDateString(fp, file.data.frontmatter.date, parseOpts) |
There was a problem hiding this comment.
Yeah I think this makes sense. Actually you can cherrypick that PR into here (I can close that one)
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)Date. Unfortunately, it is still in the proposal process and has not been implemented in any JS engines. Luxon has a similar design and is stable. We should consider switching to Temporal whenever it gets standardized and implemented in nodeAllow configurable default timezonedefaultDateTypesettingdefault changed(updated: change in default has been reverted)Some 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