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

Incorrect type determining required props #654

Closed
scottbedard opened this issue Mar 5, 2021 · 3 comments · Fixed by #655
Closed

Incorrect type determining required props #654

scottbedard opened this issue Mar 5, 2021 · 3 comments · Fixed by #655

Comments

@scottbedard
Copy link
Contributor

scottbedard commented Mar 5, 2021

I believe we have a bug with how we're determining required vs optional props...

type RequiredKeys<T> = {
[K in keyof T]: T[K] extends { required: true } | { default: any } ? K : never
}[keyof T]

The second half of this condition looks backwards. We should be flagging props as required when they extend { default: never }, not { default: any }. Having a default value means the prop isn't required, correct?

To demonstrate the problem, create a component with the following prop...

foo: {
  type: String,
  default: 'asdf',
}

Then attempt to render that component without defining foo.

Type '{}' is not assignable to type '{ foo: string; } & {}'.
Property 'foo' is missing in type '{}' but required in type '{ foo: string; }'.

As you can see, our optional prop is incorrectly ending up on the required side of this intersection type.

It looks like this bug also exists in 3.x. If this is correct and I'm not misunderstanding how default should be effecting prop types, let me know and I'll open a PR there as well!
https://github.com/vuejs/vue-next/blob/4a965802e883107a2af00301a59fb7f403b6acf7/packages/runtime-core/src/componentProps.ts#L70

scottbedard added a commit to scottbedard/composition-api that referenced this issue Mar 5, 2021
@antfu
Copy link
Member

antfu commented Mar 11, 2021

@scottbedard Looks like the approach is not correct, I am reverting it for now.

About the problem, I think we have two different props, one for outer and one for inter while your problem looks like about outer props that pass into the component instead of the props for the setup function. Can you share a snippet about how you get this error?

@scottbedard
Copy link
Contributor Author

scottbedard commented Mar 11, 2021

@antfu Ah, I did not realize that props is not the same in both contexts. I am in the process of migrating a large codebase to composition api + typescript, and we have many root components. The goal here was to build a helper for mounting those root components, while maintaining type coverage.

Here is a minimal reproduction
https://github.com/scottbedard/required-prop-types-demo

@scottbedard
Copy link
Contributor Author

scottbedard commented Mar 16, 2021

Quick update here, it looks like we don't need to make any changes. I can get the type I need by using ExtractDefaultPropTypes

type ExtractPropTypesExternal<T, U = ExtractDefaultPropTypes<T>> = Partial<U>
  & Omit<ExtractPropTypes<T>, keyof U>;

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 a pull request may close this issue.

2 participants