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

types: allow ExtractPropTypes to make props with default values optional #3125

Closed
wants to merge 1 commit into from
Closed

types: allow ExtractPropTypes to make props with default values optional #3125

wants to merge 1 commit into from

Conversation

HcySunYang
Copy link
Member

close: #3122

@HcySunYang
Copy link
Member Author

CI failure is irrelevant

@07akioni
Copy link
Contributor

07akioni commented Feb 1, 2021

#3122 (comment)

@HcySunYang
Copy link
Member Author

I read the comments, but this PR did not introduce any breaking changes, it just allows the user to do it with the way they want, just a utility for types

@07akioni
Copy link
Contributor

07akioni commented Feb 1, 2021

It may cause breaking changes.

{
  props: {
    foo: {
      type: Object as PropType<{ key: string }>,
      default: () => ({ key: 'bar' })
    }
  },
  setup (props) {
    function f (x: string) {}
    // If props.foo is optional, then break
    f(props.foo.key)
  }
}

I think if you did want do this, distinguish resolved (inside setup) & passed (from render function) version ExtractPropTypes is needed.

@HcySunYang
Copy link
Member Author

@07akioni did not break that, you should notice that MakeDefaultsOptional is false by default, which is the original behavior

@Eterion
Copy link

Eterion commented Feb 6, 2021

Is it really the best course of action to extend the ExtractPropTypes with a parameter, instead of creating new type for it?
I'm concerned about the final readability in the code, for example:

const useProps: ExtractPropTypes<typeof props, true> = {}

It's not really obvious what the second parameter actually does. I would rather see a new standalone type for it (doesn't have to be named like this).

const useProps: ExtractUsagePropTypes<typeof props> = {}

@HcySunYang HcySunYang added the 🧹 p1-chore Priority 1: this doesn't change code behavior. label Mar 19, 2021
@yyx990803 yyx990803 deleted the branch vuejs:master January 18, 2022 08:31
@yyx990803 yyx990803 closed this Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 p1-chore Priority 1: this doesn't change code behavior. scope: types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow ExtractPropTypes to make props with default values optional
4 participants