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

Adds strict type definitions to sprintf #18

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

erikyo
Copy link

@erikyo erikyo commented May 16, 2024

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

Copy link
Owner

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for submitting this so quickly. At a glance, it all looks great, and pleasantly surprised how simple it turned out.

I'll try to get this merged and published sometime this weekend, along with some other general maintenance work (dependencies, etc.).

packages/sprintf/index.js Show resolved Hide resolved
packages/sprintf/types/index.d.ts Outdated Show resolved Hide resolved
packages/sprintf/.gitignore Outdated Show resolved Hide resolved
Copy link
Owner

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works great! I was able to test it against the examples in __tests__/index.js. For whatever reason, I originally exempted __tests__ files from type-checking, so they weren't run by default. But adding // @ts-check manually to the top of the file helped in validating the types. There's a lot of type errors in the edge case handling, but I think that's expected (desired, even).

@aduth
Copy link
Owner

aduth commented May 17, 2024

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

expect(sprintf('%.*s', 3, 'abcdef')).to.equal('abc');

References:

How feasible might it be to include support for this?

@erikyo
Copy link
Author

erikyo commented May 18, 2024

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

johnhooks commented May 18, 2024

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

johnhooks commented May 18, 2024

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

@erikyo
Copy link
Author

erikyo commented May 18, 2024

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

johnhooks commented May 18, 2024

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

erikyo commented May 18, 2024

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

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 this pull request may close these issues.

Strongly typed sprintf
3 participants