feat(transloco|locale): Improve pipe type definitions for strict mode #770
Conversation
| transform( | ||
| value: string | number, | ||
| value: number | string, | ||
| numberFormatOptions?: NumberFormatOptions, | ||
| locale?: Locale | ||
| ): string; | ||
| transform( | ||
| value: null | undefined, | ||
| numberFormatOptions?: NumberFormatOptions, | ||
| locale?: Locale | ||
| ): null | undefined; | ||
| transform( | ||
| value: number | string | null, | ||
| numberFormatOptions?: NumberFormatOptions, | ||
| locale?: Locale | ||
| ): string | null; | ||
| transform( | ||
| value: number | string | undefined, | ||
| numberFormatOptions?: NumberFormatOptions, | ||
| locale?: Locale | ||
| ): string | undefined; | ||
| transform( | ||
| value: number | string | null | undefined, | ||
| numberFormatOptions?: NumberFormatOptions, | ||
| locale?: Locale | ||
| ): string | null | undefined; | ||
|
|
||
| transform( | ||
| value?: string | number | null, |
There was a problem hiding this comment.
The overloading seems robust.
Why not simply:
transform(
value: string | number,
numberFormatOptions?: NumberFormatOptions,
locale?: Locale
): string;
transform<T extends undefined | null>(
value: T,
numberFormatOptions?: NumberFormatOptions,
locale?: Locale
): T;
transform(
value: string | number | undefined | null,
numberFormatOptions: NumberFormatOptions = {},
locale?: Locale
)Seems like it covers the cases correctly.
This goes for all the pipes and overloading.
LMK if you find something that breaks 🙂
There was a problem hiding this comment.
Don't know why I didn't think of this. I adjusted the commits, thanks.
Kaemmelot
force-pushed
the
pipe-type-definitions
branch
2 times, most recently
from
85711f6
to
0a1c879
Compare
|
@Kaemmelot Did you happen to see my comment? |
Keep null and undefined intact and provide overloads with null and undefined values for all pipes. BREAKING CHANGE: 🧨 Pipes return null or undefined instead of an empty string if the input is null or undefined ✅ Closes: jsverse#755
Kaemmelot
force-pushed
the
pipe-type-definitions
branch
from
0a1c879
to
70338af
Compare
|
@shaharkazaz I saw your comment and was about to change the commits. Since this is the first time I wanted to rebase changes with GitHub as a tool, I tried the Sync button in my fork. That added an unwanted merge commit. Guess I'm not clicking that again and continue with Git and force push. Haven't used GitHub that much, some parts of the UI are a bit confusing I think. |
| locale?: Locale | ||
| ): T; | ||
| transform<T extends null | undefined>( | ||
| value: number | string | T, |
There was a problem hiding this comment.
My suggestion didn't work?
There was a problem hiding this comment.
Not sure if I understood you correctly. I guess you meant to remove the last overload? If I would remove this overload from this pipe and pass in a string | null value, I would get this error:
No overload matches this call.
Overload 1 of 2, '(value: string | number, display?: CurrencyDisplayType | undefined, numberFormatOptions?: NumberFormatOptions | undefined, currencyCode?: string | undefined, locale?: string | undefined): string', gave the following error.
Argument of type 'string | null' is not assignable to parameter of type 'string | number'.
Type 'null' is not assignable to type 'string | number'.
Overload 2 of 2, '(value: null | undefined, display?: CurrencyDisplayType | undefined, numberFormatOptions?: NumberFormatOptions | undefined, currencyCode?: string | undefined, locale?: string | undefined): null | undefined', gave the following error.
Argument of type 'string | null' is not assignable to parameter of type 'null | undefined'. ts(2769)
transloco-currency.pipe.ts(56, 3): The call would have succeeded against this implementation, but implementation signatures of overloads are not externally visible.
So I think I need all three overloads.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently the pipes don't provide overloads that can return null or undefined if provided in the input. The locale pipes transform these values into empty strings, which makes
value | pipe ?? elseValueuse cases hard. The typings are also not helpful for strict mode.Issue Number: #755
What is the new behavior?
The pipes now return null or undefined as given in the input and provide method overloads to keep the typings intact for strict mode.
Does this PR introduce a breaking change?
These changes could potentially be breaking in some cases as the locale pipes now return null or undefined where they previously returned an empty string instead. The transloco pipe has new return types but returns the same values as previously, so I don't think that pipe has a breaking change.
Other information
The order of the overloads is important, because typescript tries to use the first matching overload. The docs can be seen for every overload if provided on the first one, so they must be on top.