I18N: Use @tannin/sprintf
#70434
I18N: Use @tannin/sprintf
#70434
Conversation
|
Continuing on what we discussed what I meant was that TranslatableText is return type for all translate function and we use the Extract and sprintf definitions in the sprintf.ts file aka the whole i18n pkg needs to be written in TypeScript where each translate fn gets there type updated to use TranslatableText in return type and we update sprintf to allow working with it. so for example the __ type can be updated to export type __ = < T extends string >(
text: T,
domain?: string
) => TranslatableText< T >;also after trying somewhat more We can remove ExtractT and only do /**
* Returns a formatted string. If an error occurs in applying the format, the
* original format string is returned.
*
* @template {string} T
* @param {T | TranslatableText<T>} format The format of the string to generate.
* @param {SprintfArgs<T>} args Arguments to apply to the format.
*
* @see https://www.npmjs.com/package/sprintf-js
*
* @return {string} The formatted string.
*/
export function sprintf< T extends string >(
format: T | TranslatableText< T >,
...args: SprintfArgs< T >
): string;
export function sprintf< T extends string >(
format: T | TranslatableText< T >,
args: SprintfArgs< T >
): string;
export function sprintf< T extends string >(
format: T | TranslatableText< T >,
...args: SprintfArgs< T >
): string {
try {
return sprintfjs( format as T, ...( args as SprintfArgs< T > ) );
} catch ( error ) {
if ( error instanceof Error ) {
logErrorOnce( 'sprintf error: \n\n' + error.toString() );
}
return format;
}
}and it works for acceptable cases ( except the one with wrong type in arg ) cc: @swissspidy |
e87c1ea to
b8b2e39
Compare
| } catch ( error ) { | ||
| if ( error instanceof Error ) { | ||
| logErrorOnce( 'sprintf error: \n\n' + error.toString() ); | ||
| } | ||
| return format; | ||
| } |
There was a problem hiding this comment.
@tannin/sprintf just silently returns the original string without throwing any errors.
I think I made it work with just JSDoc alone. Though it would be nice to rewrite in TS eventually. |
|
Size Change: -1.33 kB (-0.07%) Total Size: 1.87 MB
ℹ️ View Unchanged
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
packages/i18n/src/create-i18n.js
Outdated
| * | ||
| * Retrieve translated string with gettext context. | ||
| * | ||
| * @see https://developer.wordpress.org/reference/functions/_x/ | ||
| */ | ||
| /** | ||
| * @typedef {(single: string, plural: string, number: number, domain?: string) => string} _n | ||
| * @typedef {<T extends string>(single: T, plural: T, number: number, domain?: string) => import('./types').TranslatableText< T >} _n |
There was a problem hiding this comment.
I think, this might fail when the placeholders are different for single and plural.
For example:
// translators: %1$sd and %2$d are greetings to be displayed in the message.
sprintf( _n( '%2$d hi %1$s', '%1$s hi %2$d', 1 ), 'hi', 'hi' );It should normally fail but it doesn't as both plural and single are using same generic, it adds an OR for both which causes the 2nd args to be any/unknown
Possible solution that I was able to come up with is:
type BuildTuple<
L extends number,
T extends unknown[] = [],
> = T[ 'length' ] extends L ? T : BuildTuple< L, [ unknown, ...T ] >;
type IsGreaterThan1< N extends number > = BuildTuple< N > extends [
unknown,
unknown,
// eslint-disable-next-line @typescript-eslint/no-unused-vars
...infer _Rest,
]
? true
: false;
export type _n = < T extends string, P extends string, N extends number >(
single: T,
plural: P,
number: N,
domain?: string
) => TranslatableText< IsGreaterThan1< N > extends true ? P : T >;There was a problem hiding this comment.
Interesting. I'll need to check it out a bit more closely.
At first glance: IsGreaterThan1 is a fallacy that doesn't work for every language. Plus the number is likely not known anyway (could be arr.length or so).
There was a problem hiding this comment.
That does make sense, and yeah the number inside a generic function does cause TS to not know the value and defaults to single.
I will try to experiment a bit more to see what can be done so it is usable for dynamic cases.
There was a problem hiding this comment.
That would be amazing, thanks for your help!
Worst case, if the OR for both causes some errors, it would be up to the consumer to do any code fixes or ignore any errors. As long as that is documented, it would be fine by me.
There was a problem hiding this comment.
I think I made it work.
type DistributeSprintfArgs< T extends string > = T extends any
? SprintfArgs< T >
: never;and replacing SprintfArgs<T> with this new DistributeSprintfArgs<T> in sprintf,
export function sprintf< T extends string >(
format: T | TranslatableText< T >,
...args: DistributeSprintfArgs< T >
): string;
export function sprintf< T extends string >(
format: T | TranslatableText< T >,
args: DistributeSprintfArgs< T >
): string;
export function sprintf< T extends string >(
format: T | TranslatableText< T >,
...args: DistributeSprintfArgs< T >
): string {
//...
}it will distribute unions and we get an OR of both SprintfArgs<T> and SprintfArgs<P> ( or only T if both single and plural are type of T ( tho I prefer them to be separated, looks better on intellisense ) ).
Then we won't be reallying on the number part of _n and will give us a decent typing to work with.
Screenshot
cc: @swissspidy
packages/i18n/src/create-i18n.js
Outdated
| * | ||
| * Translates and retrieves the singular or plural form based on the supplied | ||
| * number. | ||
| * | ||
| * @see https://developer.wordpress.org/reference/functions/_n/ | ||
| */ | ||
| /** | ||
| * @typedef {(single: string, plural: string, number: number, context: string, domain?: string) => string} _nx | ||
| * @typedef {<T extends string>(single: T, plural: T, number: number, context: string, domain?: string) => import('./types').TranslatableText< T >} _nx |
|
I may have missed something, but will someone please explain My biggest concern here is that this type is now exposed to consumers as the return type of all the I see part of this comment but I'm not sure where this discussion can be followed:
|
Sure, this was at aduth/tannin#15 (comment) Basically, the new types work fine for |
|
Glad to see this land 🎉 Already with the changes included in this pull request, the strong typing is already looking to prove itself quite valuable. Major props to all the work along the way from aduth/tannin#15, @swissspidy @erikyo @USERSATOSHI 🙌 |
|
Nice, thank you for taking over the prototype I started and bringing it to completion. That's going to be a nice win! |
|
Thanks Pascal ❤️ |
|
While I agree this was a good change with stricter types, this change does mean that This has surfaced at least one fatal in the wild. |
Hmm, This could be fixed by either adding an optional chaining to What do you think would be better solution? |
If I understand, the issue is specifically with an
Likely not a very common case since presumably someone wanting to format a string has a string to work with, but possible in a scenario where passing through some variable to |
Yes, exactly. |
|
I also agree with having stricter types, though we just encountered an issue that
|
|
I think this change may have broken the React Native codebase. When I try running it now I see this error: error: Error: While trying to resolve module |
hi @talldan, wanted to confirm something, is the react-native environment configured to support typescript files? As I believe the migration of i18n to typescript converted src folder to typescript and now I am seeing that in package.json, react-native is pointed to src/index which I missed while checking package.json main and module when migrating ( sorry for that mistake on my side ) |
|
Another thought is that the package only defines |
|
For more context here's the metro build error: LogMetro has encountered an error: While trying to resolve module `@tannin/sprintf` from file `/Users/me/projects/gutenberg/packages/i18n/src/sprintf.ts`, the package `/Users/me/projects/gutenberg/node_modules/@tannin/sprintf/package.json` was successfully found. However, this package itself specifies a `main` module field that could not be resolved (`/Users/me/projects/gutenberg/node_modules/@tannin/sprintf/index`. Indeed, none of these files exist:
* /Users/me/projects/gutenberg/node_modules/@tannin/sprintf/index(.ios.js|.native.js|.js|.ios.cjs|.native.cjs|.cjs|.ios.json|.native.json|.json|.ios.scss|.native.scss|.scss|.ios.sass|.native.sass|.sass|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx)
* /Users/me/projects/gutenberg/node_modules/@tannin/sprintf/index/index(.ios.js|.native.js|.js|.ios.cjs|.native.cjs|.cjs|.ios.json|.native.json|.json|.ios.scss|.native.scss|.scss|.ios.sass|.native.sass|.sass|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx): /Users/ramon/projects/gutenberg/node_modules/metro/src/node-haste/DependencyGraph.js (283:17)
281 | }
282 | if (error instanceof InvalidPackageError) {
> 283 | throw new PackageResolutionError({
| ^
284 | packageError: error,
285 | originModulePath: from,
286 | targetModuleName: to,
RCTFatal
__28-[RCTCxxBridge handleError:]_block_invoke
_dispatch_call_block_and_release
_dispatch_client_callout
_dispatch_main_queue_drain.cold.7
_dispatch_main_queue_drain
_dispatch_main_queue_callback_4CF
__CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__
__CFRunLoopRun
CFRunLoopRunSpecific
GSEventRunModal
-[UIApplication _run]
UIApplicationMain
__debug_main_executable_dylib_entry_point
start_sim
0x0 |
Sorry for the delay replying. I don't know, I've not personally worked on the native code much, I only noticed the error when testing some changes to react native files. |
Got it, I will test it on my side and see any solutions/fixes for this. |
What?
Closes #52985
Replaces the
sprintf-jslibrary with the leaner@tannin/sprintflibrary which has much stronger types. We already use the maintanninlibrary for i18n anyway, so this is a great companion.Why?
How?
Testing Instructions
Tests should all pass, TypeScript check should pass too.
Testing Instructions for Keyboard
Screenshots or screencast