-
-
Notifications
You must be signed in to change notification settings - Fork 236
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.
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
feat: Support ISO 8601 date strings with full precision for all formatting functions where dates can be passed #758
base: main
Are you sure you want to change the base?
Conversation
@martinmunillas is attempting to deploy a commit to the next-intl Team on Vercel. A member of the Team first needs to authorize it. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting the ball rolling here! I currently have some concern about this and would like the leave this open for now, please see the inline comment.
@@ -5,7 +5,8 @@ export enum IntlErrorCode { | |||
INSUFFICIENT_PATH = 'INSUFFICIENT_PATH', | |||
INVALID_MESSAGE = 'INVALID_MESSAGE', | |||
INVALID_KEY = 'INVALID_KEY', | |||
FORMATTING_ERROR = 'FORMATTING_ERROR' | |||
INVALID_FORMAT = 'INVALID_FORMAT', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the existing FORMATTING_ERROR
would do for this and avoids increasing the bundle size more than necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so at first but then I thought FORMATTING_ERROR was a formatting error but not an error because of the format while parsing. but if it still makes sense to you I'll revert it
@@ -153,6 +153,22 @@ export default function createFormatter({ | |||
} | |||
} | |||
|
|||
if (typeof value === 'string') { | |||
const str = value; | |||
value = new Date(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two problems that I know of with date strings:
1) Optional parts of date strings
Various components can be omitted, so the following are all valid:
- Date-only form: YYYY, YYYY-MM, YYYY-MM-DD
- Date-time form: one of the above date-only forms, followed by T, followed by HH:mm, HH:mm:ss, or HH:mm:ss.sss. Each combination can be followed by a time zone offset.
(source)
Omitting the time or the time zone part in the input is problematic because by calling the constructor, a time and time zone are implied and the user can format them:
// E.g. "1:00:00 AM Central European Standard Time"
formatter.dateTime('2020-11-20', {timeStyle: 'full'})
The time zone is especially tricky due to:
When the time zone offset is absent, date-only forms are interpreted as a UTC time and date-time forms are interpreted as local time. This is due to a historical spec error that was not consistent with ISO 8601 but could not be changed due to web compatibility. See Broken Parser – A Web Reality Issue.
Here's an interesting case for you that can be run in the test suite of this repo (uses TZ=Europe/Berlin
in the Node.js environment when running a test):
const formatter = createFormatter({locale: 'en'});
expect(
formatter.dateTime('2020-11-20', {
dateStyle: 'medium',
timeZone: 'America/New_York'
})
).toBe('Nov 20, 2020'); // ❌ Result: Nov 19, 2020
Fun, right? The reason is that the UTC time zone is assumed and the explicitly provided time zone moves the created date the the previous date.
2) Using non-standard date strings
The date constructor also accepts non-standard date strings with varying browser support.
Maybe due to this mess Intl.DateTimeFormat.prototype.format()
only accepts Date | number
. Hopefully Temporal
will become the better alternative at some point in the future.
Don't get me wrong, I absolutely see your point and am trying to optimize for convenience with next-intl
. By moving the date parsing into userland, we currently at least make it obvious how a string is turned into a date. Some users might prefer to lint against this even.
I think to move forward with this, we'd have to validate that the incoming date string conforms to ISO 8601 and includes all parts (year, month, date, hour, minute, seconds, milliseconds, timezone). That however will increase the bundle size slightly.
I'm honestly not confident about this change currently. Especially Intl.DateTimeFormat.prototype.format()
not accepting strings makes me suspicious, maybe there are other problems I don't know about yet.
I think as an immediate todo, the next-intl
docs should explain these problems in more detail in the Formatting dates and times docs. Currently, there's an expandable section "How can I parse dates or manipulate them?" there. I think it should be split in two and details about parsing should be included.
I'm not saying this will never make it in, but I'd say we should leave the PR open for a bit, give it more thought and maybe consider opinions from other users in case others chime in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand point 1 but I don't see how that is any different to what exists now, if i create a date with new Date(2020, 11, 20)
and format that, the error is the same, is not an issue of string parsing but on how the date object is created, if the date doesn't contain a
About point 2, yes, there is not much to argue there, js Dates are a mess, and there could be possibles bugs as everywhere, but this doesn't change anything for users use other parsing mechanisms, only adds an easier way for users that do use the new Date(str)
one. I wouldn't also mind to include other date parser if necessary or change the error message and documentation if necessary.
I still understand your concerns and appreciate your feedback! I'm just think it would be super useful to directly accept strings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if i create a date with new Date(2020, 11, 20) and format that, the error is the same
That's true. To me, the difference is that this is in userland and hopefully caught in review (or by a linter).
If the user calls format.dateTime('2020-11-20', {dateStyle: 'medium'})
(from my example above) this looks correct. Even the output "Nov 19, 2020" looks correct—but is off by a day. In this simplified example it's easy to spot but maybe less so in a complex app.
The possibility I see here is validating at runtime that a full ISO 8601 string is passed. I'm unsure if dev-only error handling is a good idea here, and I'd like to avoid increasing the bundle size due to this. We currently don't have this issue if the user calls new Date
in the app code. Furthermore, this gives the user the chance to include runtime validation code if it's relevant for the app.
I think for the time being I'll wait with this and instead improve the docs on parsing dates. I hope you can understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A closely related note from the recently released date-fns@3
:
(https://blog.date-fns.org/v3-is-out/)
Example:
Might be worth keeping an eye on this, maybe it works out well for date-fns
or there are learnings for next-intl
here.
For reference, here's a blog post on a 2.0 prerelease for date-fns
with more background on string parsing.
This is the implementation of toDate
from date-fns
that is used for normalizing dates internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another related update: Tempo was just released and they support receiving dates as strings. They include runtime validation that will throw in case an invalid date string is encountered. I've asked the library author for some background on this decision.
Maybe in the end we should really support this pattern. I'd still consider doing the runtime validation only during development to avoid increasing the bundle size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I just saw that zod
has built-in ISO 8601 parsing. They reference a StackOverflow answer which seems like it could serve us well here: https://stackoverflow.com/a/3143231/343045 (the "complete precision" variant).
We should be sure that we have good test coverage on which strings will print the warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a second thought, maybe we don't need to get too elaborative with docs and explain all the problems with dates in JavaScript.
Maybe it'd be sufficient to not add the troubleshooting section and use this for the error message:
Invalid ISO 8601 date string received: ${value}. Note that all parts of ISO 8601 are required: year, month, date, hour, minute, seconds, milliseconds and the timezone (e.g. '2024-02-21T07:11:36.398Z').
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @amannn sorry for leaving this a little abandoned, I haven't really had time to get my hands on this again. I'm not sure when I'll be able to, so anyone feel free to take over if they have time, if not I'll finish this, I just don't know when.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the implementation is good now. One tradeoff I noticed: Dates in messages are required to be actual dates, strings will lead to a formatting error. So there's some slight divergence there.
Potentially with #705 we'd have more control over this in the future and could enable string dates in messages too.
Need to decide between:
- Support this silently for the time being
- Suggest this in the docs (potentially with people being confusing why they can't use date strings with
t
) - Wait for feat: AOT compilation with
icu-to-json
(experiment) #705 - Ask for upstream support in
intl-messageformat
…ring-support # Conflicts: # packages/use-intl/src/core/createFormatter.tsx # packages/use-intl/test/core/createFormatter.test.tsx
2df60a7
to
8bbaaa7
Compare
This PR adds support to format ISO 8601 directly, this is super useful for date strings coming directly from the backend, it makes sense as it is the official format accepted in js.
As of right now I find myself doing
format.dateTime(new Date(str), format)
which is pretty annoying.I'm not sure if my changes are enough but I assume it should work, please provide any feedback.
Example
Todo