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

fix(runtime-core): return boolean type when use defineProps to set optional boolean prop #7116

Closed
wants to merge 9 commits into from

Conversation

godxiaoji
Copy link
Contributor

@godxiaoji godxiaoji commented Nov 12, 2022

fix #5847 fix #7487

The results are as follows:

const props = defineProps<{
  str?: string;
  bool?: boolean;
}>();
// readonly props.str?: string | undefined
// readonly props.bool: boolean


const propsWithDefaults = withDefaults(
  defineProps<{
    str?: string;
    str1: string;
    bool?: boolean;
    bool1: boolean;
  }>(),
  {
    str: "msg",
    str1: undefined,
    bool: undefined,
    bool1: undefined,
  }
);
// readonly props.str: string
// readonly props.str1?: string | undefined
// readonly props.bool?: boolean | undefined
// readonly props.bool1?: boolean | undefined

@netlify
Copy link

netlify bot commented Nov 12, 2022

Deploy Preview for vuejs-coverage failed.

Name Link
🔨 Latest commit bffb1b2
🔍 Latest deploy log https://app.netlify.com/sites/vuejs-coverage/deploys/63bfa0614bded20008845285

@@ -13,16 +13,20 @@ describe('defineProps w/ type declaration', () => {
// type declaration
const props = defineProps<{
foo: string
bool?: boolean
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider this case

      boolAndUndefined: boolean | undefined

{
[K in keyof TypeProps as TypeProps[K] extends boolean | undefined
? K
: never]-?: TypeProps[K]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: never]-?: TypeProps[K]
: never]-?: NotUndefined<TypeProps[K]>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without NotUndefined, the '-?' will still remove 'undefined'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it won't when there's only boolean | undefined and no ?:

interface T {
  boolAndUndefined: boolean | undefined
}

type Test = {
  [K in keyof T]-?: T[K]
}
// type Test = {
//     boolAndUndefined: boolean | undefined;
// }

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgCrIN4ChnIEYD2BANgIIgAmAqpRDKBBQFz5HERwjIA+yArrXohGWAL5YsYAJ4AHFKggBnMMgC8mHMgDaAaWShkAawhSCMNAF0AtAH4WqXRbFYgA

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, modified.

}>()
// explicitly declared type should be refined
expectType<string>(props.foo)
// the Boolean absent props will be cast to false
expectType<boolean>(props.bool)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation will lead it cannot jump to the declaration by clicking bool of props.bool (props.foo also).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create a new PR with another implementation.

@yyx990803
Copy link
Member

@godxiaoji Thank you very much for the PR - @sxzz came up with a better alternative in #7619 that retains the ability to jump to definition on props properties, but we really appreciate your contribution!

@godxiaoji
Copy link
Contributor Author

@godxiaoji Thank you very much for the PR - @sxzz came up with a better alternative in #7619 that retains the ability to jump to definition on props properties, but we really appreciate your contribution!

Is my pleasure. @yyx990803 I have a few pr, free also trouble to review #7133 #6827 #6773 #6765.

@godxiaoji godxiaoji deleted the fix-5847 branch February 8, 2023 00:45
zhangzhonghe pushed a commit to zhangzhonghe/core that referenced this pull request Apr 12, 2023
IAmSSH pushed a commit to IAmSSH/core that referenced this pull request May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants