-
-
Notifications
You must be signed in to change notification settings - Fork 662
IsLiteral: Adding strictStringCheck option
#1153
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
base: main
Are you sure you want to change the base?
Conversation
… of `IsStringLiteral` returning boolean for unions and `IsNumericLiteral` returning false for numeric unions - Matching `IsStringLiteral` behavior to the other is*Literals when dealing with unions of different types return boolean, now return false. - Fixing `IsNumericLiteral` return false for numeric union like (1 | 1n), now return true. - Adding `strict` option to control the behavior of `IsStringLiteral` against infinite signature types.
|
@som-sm I think this is a great way to solve the problem we encounter in |
|
@benzaria Please keep the PRs focused on a single change. In this case, just add the Keeping changes atomic makes it much easier to review and provide feedback. Hope that makes sense! |
|
@som-sm I want to point out that the issue u mentioned in the review for the boolean behavior getting removed from My thought on this is keeping the |
Ideally, all the logic should remain in export type IsStringLiteral<S, Options extends IsStringLiteralOptions = {}> =
ApplyDefaultOptions<IsStringLiteralOptions, DefaultIsStringLiteralOptions, Options> extends infer ResolvedOptions extends Required<IsStringLiteralOptions>
? IfNotAnyOrNever<S,
_IsStringLiteral<CollapseLiterals<S extends TagContainer<any> ? UnwrapTagged<S> : S>, ResolvedOptions>,
false, false>
: never;
type _IsStringLiteral<S, Options extends Required<IsStringLiteralOptions>> =
// If `T` is an infinite string type (e.g., `on${string}`), `Record<T, never>` produces an index signature,
// and since `{}` extends index signatures, the result becomes `false`.
S extends string
? Options['strict'] extends false
? LiteralChecks<S, string>
: {} extends Record<S, never>
? false
: true
: false;And, this would ensure you get |
|
@som-sm i did ur suggestion. and also later on we can modify |
5b79acc to
663365c
Compare
som-sm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benzaria Please address this #1153 (comment), this PR still seems to have changes that are not related to adding strict option to IsLiteral and IsStringLiteral.
|
This PRs will be ready to review after Merge of |
@som-sm Are u referring to the changes on |
som-sm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move Fixing unpredictable behaviors part in a separate PR.
source/is-literal.d.ts
Outdated
| By default, it's Strict only matching finite literals, See {@link IsLiteralOptions.strict strict} option to change this behaviour. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run it through AI please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the new doc ! and tell me if it any good
source/is-literal.d.ts
Outdated
| @default true | ||
| */ | ||
| strict?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strict name is ambiguous here, since IsLiteral doesn't involve just strings.
I was thinking of referencing the IsStringLiteralOptions in IsLiteralOptions, so we'd not have to repeat ourself.
type IsStringLiteralOptions = {
strict?: boolean;
};
/**
Supports all {@link IsStringLiteralOptions} options.
*/
type IsLiteralOptions = IsStringLiteralOptions;In future if we add options to any other Is*Literal type, we can simply & it here and add a link, like:
/**
Supports all {@link IsStringLiteralOptions} and {@link IsNumberLiteralOptions} options.
*/
type IsLiteralOptions = IsStringLiteralOptions & IsNumberLiteralOptions;Only downside is that we would have ensure there's no conflicting names. @sindresorhus WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sound amazing actualy. great thinking !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I Did it, and test it with and other options and worked flawlessly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sindresorhus @som-sm Hey guys what should we keep the current strictStringCheck options name as is or change it?
here are some suggestion I'm thinking of:
-
strictStringLiteral -
strictLiteralCheck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strictStringLiteral
👍
IsLiteral: Adding strict options and Fixing unpredictable behaviors…IsLiteral: Adding strictStringCheck option
Adding
strictStringCheckoption to control the behavior ofIsStringLiteralagainst wide patterns such as template literals (on${string}), or utility type results likeUppercase<string>.strictStringCheckistrue, types likeon${string}orUppercase<string>are considered not string literals.false, those cases are considered valid literals.Fixes #1145