Skip to content

Inaccurate type for return value of Intl.RelativeTimeFormat.prototype.formatToParts #46245

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

Closed
justingrant opened this issue Oct 6, 2021 · 5 comments · Fixed by #46508
Closed
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@justingrant
Copy link
Contributor

Bug Report

The types for Intl.RelativeTimeFormat.prototype.formatToParts in lib.e2020.intl.d.ts have a few issues.

  1. According to the spec (and MDN docs), either singular or plural names for units are accepted as input, but only singular names are output. The current types assume that plural names can also be output, which is incorrect. The relevant step in the spec is Step 5 in https://tc39.es/ecma402/#sec-PartitionRelativeTimePattern, where plural inputs are converted to singular before being used in the output.

  2. Also, there's a missed opportunity here to narrow the return type of this method, because for any input unit, the return unit will be the same unit, unless it's a plural in which case it's the plural unit converted to singular. (See https://tc39.es/ecma402/#sec-makepartslist.) So the return type could be a generic where unit in the output depends on unit in the input.

  3. Finally, there's another missed opportunity to narrow the return type using the type property, because according to https://tc39.es/ecma402/#sec-FormatRelativeTimeToParts and https://tc39.es/ecma402/#sec-makepartslist, if type === 'literal' then unit will be omitted, while if type is any other value, then unit will be present in the output.

I'd be happy to submit a PR to change these types. (If a PR would be welcome.)

Here's excerpts from the current relevant types in lib.e2020.intl.d.ts:

interface RelativeTimeFormatPart {
type: string;
value: string;
unit?: RelativeTimeFormatUnit;
}

formatToParts(
value: number,
unit: RelativeTimeFormatUnit,
): RelativeTimeFormatPart[];

type RelativeTimeFormatUnit =
| "year" | "years"
| "quarter" | "quarters"
| "month" | "months"
| "week" | "weeks"
| "day" | "days"
| "hour" | "hours"
| "minute" | "minutes"
| "second" | "seconds"
;

🔎 Search Terms

Intl
RelativeTimeFormat
formatToParts
plural
singular

🕗 Version & Regression Information

Introduced in #36084.

⏯ Playground Link

https://www.typescriptlang.org/play?target=99#code/MYewdgzgLgBATlAZjAvDMBTA7jAkmKAGwDoAlDQgQygEsA3DAFRoFsMAxEOF6gCgEoA3AFgAUKEiwADpQQRU8JMURceURiAAKsqBF4AWAEwAaGAHIWNMAFcoGCGaFiA9M5jkI1wlACqYGrA08lAAFhgwlMBQ1pSEMFAAnlLhcBjRcJgAJjDJcPFhMACiAMIAsgCCALT6AAyGMBDJwGKJye72Xr7+sGgARAkYsr0wAD4wvQCOMQgYcMNjvSzgofPjWBgYANarvZmUCcOj4yEg1nNHi1a2GDsQGBKZvSLi4NA5OhAA6gEhfgEKMjkyho3lmvCkqAAfDliNZuk4XpIYHCAgAudqebx-HrvOTfULYgDaNQAurDugBCZ6uGC0mAAPUZ9JcbkYSXCZnIVFoDGYbE43Go2LMMCC6BAsEoEAgNAA5mBKAAjQjhKAgeLs8weTrC4gsmBstpmfqDOAQXoisVgCURaVyhXK1Xq1oc7VY7pmYi8QwAZkMhn4YiAA

💻 Code

const rtf = new Intl.RelativeTimeFormat();
const parts = rtf.formatToParts(42, 'minutes');
// ResultUnit is the actual type returned per the ECMA-402 spec
type ResultUnit = "year" | "quarter" | "month" | "week" | "day"  | "hour" | "minute" | "second";
const partsWithUnit = parts.filter(p => p.unit);
const unit: ResultUnit = partsWithUnit[0].unit!;
//    ^^^^
// Type 'RelativeTimeFormatUnit' is not assignable to type 'ResultUnit'.
// Type '"years"' is not assignable to type 'ResultUnit'.(2322)

🙁 Actual behavior

TS error 2322

🙂 Expected behavior

No error because the unit of the return type is always singular.

@andrewbranch andrewbranch added the Needs Investigation This issue needs a team member to investigate its status. label Oct 7, 2021
@andrewbranch andrewbranch added this to the TypeScript 4.5.1 milestone Oct 7, 2021
@orta
Copy link
Contributor

orta commented Oct 8, 2021

Cool, happy to have SingularRelativeTimeUnit added and switched out where needed 👍🏻

I'm not too wild on making the functions generic to support some of the narrowing, we really want to keep the shipped libs as simple as possible because they get included in everyone's projects and historically a lot of the times we've been clever we've had to backtrack

@justingrant
Copy link
Contributor Author

Cool, happy to have SingularRelativeTimeUnit added and switched out where needed 👍🏻

OK, I'll send a PR soon.

I also noticed another Intl type-declaration issue: the input properties to the options param of the Intl.DateTimeFormat constructor are narrowly typed string literals, e.g. localeMatcher?: "best fit" | "lookup".

But the ResolvedDateTimeFormatOptions type, which should represent the same range of string literals, is typed more loosely as string. Any idea why this is? Do TS lib types generally restrict inputs but are more forgiving for outputs in order to support later platforms that can output values that old TS lib declarations don't know about? Or is this a bug and they should match?

Here's the types for reference:

TypeScript/src/lib/es5.d.ts

Lines 4392 to 4406 in 5d0d7ae

interface DateTimeFormatOptions {
localeMatcher?: "best fit" | "lookup" | undefined;
weekday?: "long" | "short" | "narrow" | undefined;
era?: "long" | "short" | "narrow" | undefined;
year?: "numeric" | "2-digit" | undefined;
month?: "numeric" | "2-digit" | "long" | "short" | "narrow" | undefined;
day?: "numeric" | "2-digit" | undefined;
hour?: "numeric" | "2-digit" | undefined;
minute?: "numeric" | "2-digit" | undefined;
second?: "numeric" | "2-digit" | undefined;
timeZoneName?: "long" | "short" | undefined;
formatMatcher?: "best fit" | "basic" | undefined;
hour12?: boolean | undefined;
timeZone?: string | undefined;
}

TypeScript/src/lib/es5.d.ts

Lines 4408 to 4423 in 5d0d7ae

interface ResolvedDateTimeFormatOptions {
locale: string;
calendar: string;
numberingSystem: string;
timeZone: string;
hour12?: boolean;
weekday?: string;
era?: string;
year?: string;
month?: string;
day?: string;
hour?: string;
minute?: string;
second?: string;
timeZoneName?: string;
}

I'm not too wild on making the functions generic to support some of the narrowing, we really want to keep the shipped libs as simple as possible because they get included in everyone's projects and historically a lot of the times we've been clever we've had to backtrack

OK, sounds reasonable, I won't include (2) in the PR. What about (3) above which is a discriminated union not a generic?

@justingrant
Copy link
Contributor Author

But the ResolvedDateTimeFormatOptions type, which should represent the same range of string literals, is typed more loosely as string. Any idea why this is? Do TS lib types generally restrict inputs but are more forgiving for outputs in order to support later platforms that can output values that old TS lib declarations don't know about? Or is this a bug and they should match?

FWIW, newer updates to Intl are inconsistent with older libs' types. Older libs (excerpted in my earlier comment) use string, but newer updates use string literal types for ResolvedDateTimeFormatOptions.

interface DateTimeFormatOptions {
formatMatcher?: "basic" | "best fit" | "best fit" | undefined;
dateStyle?: "full" | "long" | "medium" | "short" | undefined;
timeStyle?: "full" | "long" | "medium" | "short" | undefined;
dayPeriod?: "narrow" | "short" | "long" | undefined;
fractionalSecondDigits?: 0 | 1 | 2 | 3 | undefined;
}
interface ResolvedDateTimeFormatOptions {
formatMatcher?: "basic" | "best fit" | "best fit";
dateStyle?: "full" | "long" | "medium" | "short";
timeStyle?: "full" | "long" | "medium" | "short";
hourCycle?: "h11" | "h12" | "h23" | "h24";
dayPeriod?: "narrow" | "short" | "long";
fractionalSecondDigits?: 0 | 1 | 2 | 3;
}

Any idea why they are different? Should they be different?

@orta
Copy link
Contributor

orta commented Oct 12, 2021

Generally we want to use a union of string literals when it makes sense which is most of the time, but it's can be hard to do that (which means falling back to string.) For example if a new option for hour cycle is added in es2022 we don't have a way in TypeScript to safely describe that it is hourCycle?: ES2011hourCycle | "h25" when you are in es2022.

As seen in something like

type ES2018NumberFormatPartType = "literal" | "nan" | "infinity" | "percent" | "integer" | "group" | "decimal" | "fraction" | "plusSign" | "minusSign" | "percentSign" | "currency" | "code" | "symbol" | "name";
type ES2020NumberFormatPartType = "compact" | "exponentInteger" | "exponentMinusSign" | "exponentSeparator" | "unit" | "unknown";
type NumberFormatPartTypes = ES2018NumberFormatPartType | ES2020NumberFormatPartType;

Which is both correct... and incorrect.

The other is that these are contributions from a range of people, and while I gave them all big look over for 4.5, that doesn't make them all perfectly consistent and accurate (because I've barely used these APIs in production) - so we're OK to back breaking changes like string -> "h11" | "h12" | "h23" | "h24" so long as it has some history of been in for a while and isn't looking to drastically change in the future

@justingrant
Copy link
Contributor Author

Makes sense. Would you welcome a PR to change ResolvedDateTimeFormatOptions to string literals? Or should it be left as only string properties?

if a new option for hour cycle is added in es2022 we don't have a way in TypeScript to safely describe that it is hourCycle?: ES2011hourCycle | "h25" when you are in es2022.

Could you explain the problem a little more? Not sure I fully understand it. Thanks!

justingrant added a commit to justingrant/TypeScript that referenced this issue Oct 24, 2021
This commit updates the type of `RelativeTimeFormatPart` to clarify that
the `unit` prop is always singular, unlike the plural or singular values
that are accepted as inputs.

This also changes `RelativeTimeFormatPart` to be a discriminated
union type because the `unit` prop is only present if the `type` prop's
value is not "literal".

Fixes microsoft#46245
@typescript-bot typescript-bot added the Fix Available A PR has been opened for this issue label Oct 25, 2021
justingrant added a commit to justingrant/TypeScript that referenced this issue Nov 16, 2021
This commit updates the type of `RelativeTimeFormatPart` to clarify that
the `unit` prop is always singular, unlike the plural or singular values
that are accepted as inputs.

This also changes `RelativeTimeFormatPart` to be a discriminated
union type because the `unit` prop is only present if the `type` prop's
value is not "literal".

Fixes microsoft#46245
@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Dec 15, 2021
sandersn pushed a commit that referenced this issue May 10, 2022
This commit updates the type of `RelativeTimeFormatPart` to clarify that
the `unit` prop is always singular, unlike the plural or singular values
that are accepted as inputs.

This also changes `RelativeTimeFormatPart` to be a discriminated
union type because the `unit` prop is only present if the `type` prop's
value is not "literal".

Fixes #46245
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants