aduth / tannin

gettext localization library compatible with Jed-formatted locale data
MIT License
13 stars 5 forks source link

Strongly typed `sprintf` #15

Open swissspidy opened 1 year ago

swissspidy commented 1 year ago

I just stumbled upon https://hacklewayne.com/a-truly-strongly-typed-printf-in-typescript and was wondering whether a solution like this could be considered for @tannin/sprintf.

Right now with the existing JSDoc every arg needs to be a string, when in fact the values will be cast to ints and floats anyway depending on the format. A strongly typed implementation could make this more robust.

This would also help catch issues where the number of arguments doesn't match the number of placeholders.

From that article:

type Specifiers = {
    's': string,
    'd': number,
    'b': boolean,
    'D': Date
};

type Spec = keyof Specifiers;

type Values<T extends string> = 
    T extends `${infer _}%${infer K}${infer Rest}`
    ? K extends Spec
        ? [ Specifiers[K], ...Values<Rest> ]
        : Values<`${K}${Rest}`>
    : [];

declare function printf<T extends string>(format: T, ...values: Values<T>): string;

const r = printf('this is a %s and it is %d %wyears old, right?%b %D %i %f', 'Hackle', 20, true, new Date()); // ok
printf('Hello %s', 'John'); // ok
printf('Hello %s', 'John', 'Doe'); // not ok, too many arguments
printf('Hello %s', false); // not ok, wrong type

Note: this doesn't support named arguments yet.

aduth commented 1 year ago

Hey @swissspidy ! Interesting idea, I'm definitely open to it. Is it something you'd be willing to submit a pull request for?

The current JavaScript / JSDoc tooling might make it a little difficult to port the native TypeScript types, but maybe it's fine enough to have a manually-edited .d.ts for this package, especially since it's just a single function?

swissspidy commented 1 year ago

I'd need to first dig into how to support named arguments, that might take me a bit too much time.

erikyo commented 4 months ago

@swissspidy this should support named arguments for sprintf, or at least works for me.

type Specifiers = {
    's': string,
    'd': number,
    'b': boolean,
    'D': Date
};
type S = keyof Specifiers;

type ExtractNamedPlaceholders<T extends string> =
    T extends `${infer Start}%(${infer Key})${infer Spec}${infer Rest}`
        ? Spec extends S
            ? { [K in Key]: Specifiers[Spec]} & ExtractNamedPlaceholders<Rest>
            : never
        : {};

type ExtractUnnamedPlaceholders<T extends string> =
    T extends `${infer Start}%${infer Spec}${infer Rest}`
        ? Spec extends S
            ? [Specifiers[Spec], ...ExtractUnnamedPlaceholders<Rest>]
            : never
        : [];

type SprintfArgs<T extends string> =
    ExtractUnnamedPlaceholders<T> extends never
        ? [values: ExtractNamedPlaceholders<T>]
        : ExtractUnnamedPlaceholders<T>;

https://gist.github.com/erikyo/3b0a67d55b97fbe8ec7dc8d5f310e9c4

@aduth - If you want, I can manage to create a PR that add the types to tannin/sprintf (maybe with the help of @johnhooks)

swissspidy commented 3 months ago

A PR would be fantastic!

erikyo commented 3 months ago

If @aduth agrees, I'm happy to proceed. However, I want to note that this will require making some changes. Specifically, we will need to create a new file for the definitions (which are currently generated) and import them as shown below. Additionally, using the spread operator for the last argument (ES6) will help with the type definitions of the overloaded parameters

 /**
 * @template {string} T
 * @param {T} string - string printf format string
 * @param {import('./types/index.d').SprintfArgs<T>} args String arguments.
 *
 * @return {string} Formatted string.
 */
export default function sprintf(string, ...args) {
    var i = 0;

    if (Array.isArray(args[0])) {
        args = /** @type {string[]} args */ args[0];
    }
aduth commented 3 months ago

Hey, apologies for the delayed response. Very open to a pull request here. I think the additional changes you mentioned should be fine. Spread operator could be considered a breaking change if the code is otherwise still IE11 compatible, but (a) I don't think I stated this browser support anywhere, and (b) I'm ready to move on from IE11, so agreeable regardless.

I was hoping we might be able to validate the typings in a project with a lot of usage, but it looks like Gutenberg doesn't use the sprintf subpackage of this library, and uses sprintf-js instead? I can't recall if there was a reason for that, or if the plan was to be able to migrate to the Tannin package?

It looks like @swissspidy has a similar issue at https://github.com/WordPress/gutenberg/issues/52985 if it could make more sense to introduce the typings there first.

swissspidy commented 3 months ago

I‘m using it in https://github.com/GoogleForCreators/web-stories-wp if that helps.

No idea why GB doesn‘t use this package, but maybe it could migrate

erikyo commented 3 months ago

It looks like Gutenberg doesn't use the sprintf subpackage of this library

I confirm that this is true, but I have tried it in my fork of i18n, and sprintf-js and tannin are perfectly interchangeable. Additionally, @tannin/sprintf is much faster.

it could make more sense to introduce the typings there first.

Gutenberg has a complex configuration, and I prefer to do it in this repo. If Gutenberg switches to @tannin/sprintf (as I hope), it will receive the types without changing anything in the codebase ( and this is good since I18n has more comments jsdocs rather than code).

swissspidy commented 3 months ago

If Gutenberg switches to @tannin/sprintf (as I hope)

I can definitely help with that 👍