aduth / tannin

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

Adds strict type definitions to sprintf #18

Open erikyo opened 6 months ago

erikyo commented 6 months ago

As discussed in the linked issue #15 , This PR aims to add strict definitions for sprintf

Compared with the proposed modification that I showed in the related issue, I have made a small change since i noticed that also an array is accepted as argument (basically adding this SprintfArgs<T>[]).

As mentioned, it can be considered a breaking change because the overload with the spread operator (es6) is not supported by IE.

I am not able to move the imports to the beginning of the file as @johnhooks showed me recently (but maybe he could help me with that), it should make that jsdocs code block a little less ugly

should work in this way: image

close #15

aduth commented 6 months ago

Actually, one valid usage that looks to be unsupported now by these types is this one:

https://github.com/aduth/tannin/blob/b0c3be95c6d20d5dd1c3dfb97d00acfe89ce1566/packages/sprintf/__tests__/index.js#L61

References:

How feasible might it be to include support for this?

erikyo commented 6 months ago

How feasible might it be to include support for this?

Hi @aduth, ah you are right, and actually I noticed that even in this case it is not correctly strictly typed sprintf('Hello %2$s! From %1$s.', 'Andrew', 'world');

I am trying to fix it both the issues

johnhooks commented 6 months ago

The issue I see, you need to know the types that are to be extracted, if you don't have a good delimiter. And there are a large number of different specifiers.

The only reason extracting K works in the example below (which is taken from the article that inspired this exploration reference), it has the % delimiter right in front. Though I don't understand how it determines to finish the match.

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

Note it only works if we help the pattern matching by specifying the % symbol - my guess is otherwise it's hard to decide where to stop inferring K, as it's sandwiched by two other inferences with no constraints or delimiters. With % the specifier is "anchored", therefore easier to locate. reference

That article says it was inspired by react-router typing, I took a look at those.

Check this out, react router has it easy, they have super clean delimiters in the url.

Path extends `${infer L}/${infer R}`

react-router/packages/router/utils.ts

johnhooks commented 6 months ago

My assumption, to support the large number of possibilities would require a deep set of conditional type checks, accounting for each.

erikyo commented 6 months ago

T extends ${infer _}%${infer K}${infer Rest}

@johnhooks, The reason for the match is that K is a specifier from a given set of specifiers. The type inference mechanism detects which specifier is used and determines the corresponding type accordingly.

The match ends when the template literal pattern no longer finds a % followed by a valid specifier, indicating that there are no more specifiers to process in the string.

johnhooks commented 6 months ago

I see where S, which is keyof Specifier, is used in the resulting map:

{ [K in Key]: Specifiers[Spec]}

Though the infer doesn't mean it automatically extends S.

The infer is unaware what it is extracting has anything to do with S, that is checked later in a conditional.

UPDATE:

So in the more complex instances, my assumption the inference of K is too broad, and matching more than you expect.

T extends ${infer _}%${infer K}${infer Rest}
erikyo commented 6 months ago

@johnhooks, you're probably right. I'll work on improving the string matcher to better define the various cases.