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

optional() does not create an optional property on an object #375

Closed
thesunny opened this issue Jun 9, 2020 · 7 comments
Closed

optional() does not create an optional property on an object #375

thesunny opened this issue Jun 9, 2020 · 7 comments

Comments

@thesunny
Copy link
Collaborator

thesunny commented Jun 9, 2020

optional does not create an optional property on an object. It should create an optional property in order to match with TypeScript validation semantics.

const CustomText = object({
  name: string()
  age: optional(number())
})

// creates  a type like:
type UserType = {
  name: string
  age: number | undefined
}

// But it should create a type like this:
type UserType = {
  name: string
  age?: number | undefined
}

Alignment with TypeScript

The Superstruct type does not align with the TypeScript type it is returning.

As an example, this passes Superstruct validation:

describe("content-types", () => {
  it("should validate content-types", async () => {
    const User = object({
      name: string(),
      age: optional(number()),
    })
    assert({ name: "john" }, User)
  })
})

but fails the type number | undefined returned by Superstruct:

image

The red squiggles indicate the following error when hovered:

image

Use case

My use case is that I want to validate slate data types.

They are defined such that the bold property is optional. To be specific, they are optional(literal(true). In order to make sure my validations line up against my type definitions exactly, I use unit testing to make sure the two type are equal. Because the optional types show up as a union of the passed in type and undefined but do not add the ? to the property, they do not match and fail tests.

@ianstormtaylor
Copy link
Owner

ianstormtaylor commented Jun 9, 2020

Ah interesting, that's unfortunate. I didn't actually realize they were treated differently, but I agree that it should support the prop?: use case. Would you be down to make a PR?

The issue is I'm not sure how to "lift" that typing logic up from the property's struct definition into the object's, since it's only something that applies at the object level and doesn't exist at the single-value level.

@thesunny
Copy link
Collaborator Author

thesunny commented Jun 9, 2020

Hi Ian, yes I can make a PR though not sure the timeline.

Unfortunately, the lift will be a little hacky in that anything that has a union with undefined will become optional. Pragmatically, this appears to be the way SuperStruct works.

FYI, it is impossible to get optional types strictly correct due to this limitation of TypeScript microsoft/TypeScript#13195

@ianstormtaylor
Copy link
Owner

@thesunny was just running into this myself. I'm not sure there is a way to pull up the optionality with TypeScript, but if there is I'd be open to it.

But I did realize a workaround we can use:

export const Text = intersection([
  object({
    text: string(),
  }),
  partial({
    bold: literal(true),
  }),
])

@thesunny
Copy link
Collaborator Author

Oh nice! Thanks for sharing that.

To clarify my previous post, there is a way to do it by turning all unions with undefined into ? optional properties. We pick all the properties that extend undefined and then recreate them with ?. We then intersect them with the rest of the properties.

@ianstormtaylor
Copy link
Owner

Ah interesting, that seems like a decent trade off I think. Seems pretty rare that you’d explicitly want a defined undefined value to me.

@JakeElder
Copy link

I just ran in to this issue, thank you for the workaround.

Is it likely your proposed solution will make it in @thesunny? The workaround is fine, but it would be nice to be able to avoid the extra clutter in the struct definition and resulting typescript def.

image

image

Loving your work on Superstruct, it's a joy to use.

@thesunny
Copy link
Collaborator Author

@JakeElder @ianstormtaylor

This now has a PR: #390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants