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

feat: Support ISO 8601 date strings with full precision for all formatting functions where dates can be passed #758

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: Support ISO 8601 date strings with full precision for all formatting functions where dates can be passed #758

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ed74ea6
feat: support format date strings
martinmunillas Jan 5, 2024
656e701
chore: add tests
martinmunillas Jan 5, 2024
275a403
Merge remote-tracking branch 'origin/main' into feature/add-format-st…
amannn May 21, 2024
8bbaaa7
Enhance comment
amannn May 21, 2024
fa44e74
Finish feature
amannn May 21, 2024
d9d31c5
Decrease size limit
amannn May 21, 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
3 changes: 2 additions & 1 deletion packages/use-intl/src/core/IntlError.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Owner

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.

Copy link
Contributor Author

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

FORMATTING_ERROR = 'FORMATTING_ERROR',
}

export default class IntlError extends Error {
Expand Down
32 changes: 24 additions & 8 deletions packages/use-intl/src/core/createFormatter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import DateTimeFormatOptions from './DateTimeFormatOptions';
import Formats from './Formats';
import IntlError, {IntlErrorCode} from './IntlError';
import IntlError, { IntlErrorCode } from './IntlError';
martinmunillas marked this conversation as resolved.
Show resolved Hide resolved
import NumberFormatOptions from './NumberFormatOptions';
import RelativeTimeFormatOptions from './RelativeTimeFormatOptions';
import TimeZone from './TimeZone';
import {defaultOnError} from './defaults';
import { defaultOnError } from './defaults';

const SECOND = 1;
const MINUTE = SECOND * 60;
Expand All @@ -31,7 +31,7 @@ const UNIT_SECONDS: Record<Intl.RelativeTimeFormatUnit, number> = {
quarter: QUARTER,
quarters: QUARTER,
year: YEAR,
years: YEAR
years: YEAR,
} as const;

function resolveRelativeTimeUnit(seconds: number) {
Expand Down Expand Up @@ -75,7 +75,7 @@ export default function createFormatter({
locale,
now: globalNow,
onError = defaultOnError,
timeZone: globalTimeZone
timeZone: globalTimeZone,
}: Props) {
function resolveFormatOrOptions<Options>(
typeFormats: Record<string, Options> | undefined,
Expand Down Expand Up @@ -128,7 +128,7 @@ export default function createFormatter({

function dateTime(
/** If a number is supplied, this is interpreted as a UTC timestamp. */
martinmunillas marked this conversation as resolved.
Show resolved Hide resolved
value: Date | number,
value: Date | number | string,
/** If a time zone is supplied, the `value` is converted to that time zone.
* Otherwise the user time zone will be used. */
formatOrOptions?: string | DateTimeFormatOptions
Expand All @@ -140,7 +140,7 @@ export default function createFormatter({
(options) => {
if (!options?.timeZone) {
if (globalTimeZone) {
options = {...options, timeZone: globalTimeZone};
options = { ...options, timeZone: globalTimeZone };
} else {
onError(
new IntlError(
Expand All @@ -153,6 +153,22 @@ export default function createFormatter({
}
}

if (typeof value === 'string') {
const str = value;
value = new Date(value);
Copy link
Owner

@amannn amannn Jan 10, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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

Copy link
Owner

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.

Copy link
Owner

@amannn amannn Jan 29, 2024 •
edited
Loading

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:

Screenshot 2024-01-29 at 10 18 34

(https://blog.date-fns.org/v3-is-out/)

Example:

Screenshot 2024-01-29 at 10 19 15

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.

Copy link
Owner

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.

Copy link
Owner

@amannn amannn Feb 19, 2024 •
edited
Loading

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.

Copy link
Owner

@amannn amannn Feb 21, 2024 •
edited
Loading

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').

Copy link
Contributor Author

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.

Copy link
Owner

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!

Copy link
Owner

@amannn amannn May 22, 2024 •
edited
Loading

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:

  1. Support this silently for the time being
  2. Suggest this in the docs (potentially with people being confusing why they can't use date strings with t)
  3. Wait for feat: AOT compilation with icu-to-json (experiment) #705
  4. Ask for upstream support in intl-messageformat

if (isNaN(value.getTime())) {
onError(
new IntlError(
IntlErrorCode.INVALID_FORMAT,
process.env.NODE_ENV !== 'production'
? `The \`value\` string parameter does not follow a valid ISO 8601 format. For more information check: https://tc39.es/ecma262/multipage/numbers-and-dates.html#sec-date-time-string-format`
: undefined
)
);
return str;
}
}

return new Intl.DateTimeFormat(locale, options).format(value);
}
);
Expand Down Expand Up @@ -219,7 +235,7 @@ export default function createFormatter({
const value = calculateRelativeTimeValue(seconds, unit);

return new Intl.RelativeTimeFormat(locale, {
numeric: 'auto'
numeric: 'auto',
}).format(value, unit);
} catch (error) {
onError(
Expand All @@ -238,5 +254,5 @@ export default function createFormatter({
);
}

return {dateTime, number, relativeTime, list};
return { dateTime, number, relativeTime, list };
}
68 changes: 40 additions & 28 deletions packages/use-intl/test/core/createFormatter.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,62 +1,74 @@
import {parseISO} from 'date-fns';
import {it, expect} from 'vitest';
import {createFormatter} from '../../src';
import { parseISO } from 'date-fns';
import { it, expect } from 'vitest';
import { createFormatter } from '../../src';

it('formats a date and time', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.dateTime(parseISO('2020-11-20T10:36:01.516Z'), {
dateStyle: 'medium'
dateStyle: 'medium',
})
).toBe('Nov 20, 2020');
});

it('formats a ISO 8601 datetime string', () => {
const formatter = createFormatter({
martinmunillas marked this conversation as resolved.
Show resolved Hide resolved
locale: 'en',
timeZone: 'Europe/Berlin',
});
expect(
formatter.dateTime('2020-11-20T10:36:01.516Z', {
dateStyle: 'medium',
})
).toBe('Nov 20, 2020');
});

it('formats a number', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(formatter.number(123456)).toBe('123,456');
});

it('formats a bigint', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(formatter.number(123456789123456789n)).toBe('123,456,789,123,456,789');
});

it('formats a number as currency', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.number(123456.789, {style: 'currency', currency: 'USD'})
formatter.number(123456.789, { style: 'currency', currency: 'USD' })
).toBe('$123,456.79');
});

it('formats a bigint as currency', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.number(123456789123456789n, {
style: 'currency',
currency: 'USD'
currency: 'USD',
})
).toBe('$123,456,789,123,456,789.00');
});

it('formats a relative time with the second unit', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.relativeTime(
Expand All @@ -69,7 +81,7 @@ it('formats a relative time with the second unit', () => {
it('formats a relative time with the minute unit', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.relativeTime(
Expand All @@ -82,7 +94,7 @@ it('formats a relative time with the minute unit', () => {
it('formats a relative time with the hour unit', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.relativeTime(
Expand All @@ -95,7 +107,7 @@ it('formats a relative time with the hour unit', () => {
it('formats a relative time with the day unit', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.relativeTime(
Expand All @@ -108,7 +120,7 @@ it('formats a relative time with the day unit', () => {
it('formats a relative time with the month unit', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.relativeTime(
Expand All @@ -121,7 +133,7 @@ it('formats a relative time with the month unit', () => {
it('formats a relative time with the year unit', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.relativeTime(
Expand All @@ -134,7 +146,7 @@ it('formats a relative time with the year unit', () => {
it('supports the future relative time', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.relativeTime(
Expand All @@ -147,25 +159,25 @@ it('supports the future relative time', () => {
it('formats a relative time with options', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.relativeTime(parseISO('2020-11-20T08:30:00.000Z'), {
now: parseISO('2020-11-20T10:36:00.000Z'),
unit: 'day'
unit: 'day',
})
).toBe('today');
});

it('supports the quarter unit', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.relativeTime(parseISO('2020-01-01T00:00:00.000Z'), {
now: parseISO('2020-11-01T01:00:00.000Z'),
unit: 'quarter'
unit: 'quarter',
})
).toBe('3 quarters ago');
});
Expand All @@ -174,33 +186,33 @@ it('formats a relative time with a globally defined `now`', () => {
const formatter = createFormatter({
locale: 'en',
now: parseISO('2020-11-20T01:00:00.000Z'),
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.relativeTime(parseISO('2020-11-20T00:00:00.000Z'), {
unit: 'day'
unit: 'day',
})
).toBe('today');
});

it('formats a list', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.list(['apple', 'banana', 'orange'], {type: 'disjunction'})
formatter.list(['apple', 'banana', 'orange'], { type: 'disjunction' })
).toBe('apple, banana, or orange');
});

it('formats a set', () => {
const formatter = createFormatter({
locale: 'en',
timeZone: 'Europe/Berlin'
timeZone: 'Europe/Berlin',
});
expect(
formatter.list(new Set(['apple', 'banana', 'orange']), {
type: 'disjunction'
type: 'disjunction',
})
).toBe('apple, banana, or orange');
});
Loading