-
-
Notifications
You must be signed in to change notification settings - Fork 626
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
types: support readonly defaultNS #2123
Conversation
is this ok @marcalexiei ? |
@@ -12,4 +12,4 @@ export type $OmitArrayKeys<Arr> = Arr extends readonly any[] ? Omit<Arr, keyof a | |||
|
|||
export type $PreservedValue<Value, Fallback> = [Value] extends [never] ? Fallback : Value; | |||
|
|||
export type $NormalizeIntoArray<T extends unknown | unknown[]> = T extends unknown[] ? T : [T]; | |||
export type $NormalizeIntoArray<T extends unknown | readonly unknown[]> = T extends readonly unknown[] ? T : [T]; |
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.
With this fix when using a normal array as defaultNS
the types won't work.
You can try out removing Readonly<...>
from the `CustomTypeOptions.defaultNS.
You have to take into account both array types.
This should do the trick:
export type $NormalizeIntoArray<T extends unknown | readonly unknown[]> = T extends readonly unknown[] ? T : [T]; | |
export type $NormalizeIntoArray<T extends unknown | readonly unknown[] | unknown[]> = T extends | |
| unknown[] | |
| readonly unknown[] | |
? T | |
: [T]; |
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.
What version of TS did you test this with? I tested both variants and they seem to work. See this playground https://www.typescriptlang.org/play?#code/C4TwDgpgBAkgzgQQE5IIYgDwBUB8UC8UWUEAHsBAHYAmcUSEq1A9pQDYhQCulA1pcwDulANoBdKAH4owJF2gAuKADNUbOBABQmgMas4weo2oB5dp0IiAjABoATDYDME1HT2UDu-YYFIAtmoEUNb2TmLaoJBQqEHwyGiYkRDMykZMZhw4mgD02VBQAHqSmklQAEaxiCjoGEkpUL4BbFm5+UXaOXkAtD29ff39JeDQWBAGQfE1PPxClHhkFDR0AErGrByTmNMCwnjSsvJQSqrqWq2FkkA
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.
You are right, sorry.
I tested this on my local branch but probably I had something on my working copy that broke the test 😅
@adrai LGTM |
thank you both... |
Follow-up of #2121 to also allow defaultNS to be readonly (This is the case when using
as const
on the defaultNS definitions and using typeof defaultNS in the types)Checklist
npm run test
Checklist (for documentation change)