-
-
Notifications
You must be signed in to change notification settings - Fork 231
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
refactor: deduplicate schema optional types #2788
refactor: deduplicate schema optional types #2788
Conversation
|
@@ -1850,134 +1850,85 @@ export const optionalToOptional = <FA, FI, FR, TA, TI, TR>( | |||
) | |||
) | |||
|
|||
type OptionalOptions<A> = { |
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.
Not a huge fan of the name. Tell me if you have a better idea.
@@ -704,7 +704,7 @@ S.Struct({ | |||
}) | |||
|
|||
// $ExpectType Struct<{ a: PropertySignature<":", "a" | "b", never, "?:", "a" | "b", true, never>; }> | |||
S.Struct({ a: S.optional(S.Literal("a", "b"), { default: () => "a", exact: true }) }) | |||
S.Struct({ a: S.optional(S.Literal("a", "b"), { default: () => "a" as const, exact: true }) }) |
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.
Not completely sure why it was needed but as it was done for the other overload, I thought it was fine to add as const
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'd revert this change, it's good to know that as const
is not needed
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.
ah, sorry, I read it wrong. I thought it was done for consistency, not because it was causing a new error. ok, let me see what could be causing it...
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 tries a few in
or out
in the OptionalOptions
type but without success
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.
The issue seems related to default
being a lazy arg () => A
I have an idea to solve the issue, but it involves quite a refactoring. I'll send another PR to replace this one
Closing in favour of #2794 |
Type
Description
Followup of this comment on a previous P.R.
I did not add any changeset as it's supposed to be an internal refactor but, as shown in the update of the types' tests, there's a small potential external change so tell me if you think I should add one.
Related