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.

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

Strongly typed sprintf #15

Open
swissspidy opened this issue Jul 20, 2023 · 9 comments · May be fixed by #18
Open

Strongly typed sprintf #15

swissspidy opened this issue Jul 20, 2023 · 9 comments · May be fixed by #18

Comments

@swissspidy
Copy link

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
Copy link
Owner

aduth commented Jul 25, 2023

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
Copy link
Author

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

@erikyo
Copy link

erikyo commented May 8, 2024

@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
Copy link
Author

A PR would be fantastic!

@erikyo
Copy link

erikyo commented May 15, 2024

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
Copy link
Owner

aduth commented May 15, 2024

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 WordPress/gutenberg#52985 if it could make more sense to introduce the typings there first.

@swissspidy
Copy link
Author

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
Copy link

erikyo commented May 16, 2024

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
Copy link
Author

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

I can definitely help with that 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants