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

TypeScript's defineProps macro does not properly map to the object version in the case of booleans #8576

Open
matthew-dean opened this issue Jun 14, 2023 · 12 comments
Labels
✨ feature request New feature or request

Comments

@matthew-dean
Copy link

matthew-dean commented Jun 14, 2023

What problem does this feature solve?

Vue has an unexpected behavior where if you do this:

props: {
  foo: {
    type: Boolean,
    required: false
  }
}

...the value of foo, if not specified, will be false instead of undefined. This is somewhat unexpected, as noted in vuejs/vue#7646 and vuejs/vue#4792, but my guess is that this was a conscious choice by Vue to imitate the behavior certain HTML values, at least their values in the DOM.

The problem comes in the TypeScript extension of Vue. Say you have this:

<script setup lang="ts">
interface Props {
  foo?: boolean
}
defineProps<Props>()
</script>

TypeScript is very clear about interface syntax, which is that foo?: boolean means that the value of foo will be undefined if it is not specified. The type, in fact, is not boolean but boolean | undefined from TypeScript's perspective.

Meaning, the most accurate translation of TypeScript to Vue's JavaScript API when passing an interface would be:

props: {
  foo: {
    type: Boolean,
    required: false,
    default: undefined
  }
}

I would consider this a bug of not interpreting a TypeScript interface accurately, BUT it could be also considered a feature request as supporting optional properties in TypeScript, which may have not been fully implemented.

What does the proposed API look like?

Ideally, the API would be identical:

<script setup lang="ts">
interface Props {
  foo?: boolean
}
defineProps<Props>()
</script>

However, it's rational that there would be concern about breaking changes, so this is what I propose, which is a compile-time error vs a possible runtime change:

  1. If a TypeScript type is passed into withDefaults(defaultProps<Props>, {}), any boolean type should be required in the withDefaults object, so that the developer can make it clear if they are intending the default Vue behavior, or the default TypeScript behavior.
  2. Ideally, a withDefaults macro would be required with a defaultProps macro if, again, there are boolean types in the interface, but that could be considered too high of a burden for developers.

So, my proposal, at minimum, is that the provided script block throws an error (or I can set a TypeScript strictness check to throw an error?). Thanks for considering this.

@yyx990803
Copy link
Member

FYI, the return type of defineProps does take this into account:

interface Props {
  foo?: boolean
}
const p = defineProps<Props>()
//    ^ { foo: boolean }
 p.foo
// ^ boolean

The interface declares foo as being optional, but it is not the type of the resolved runtime props. The resolved props type correctly excludes the undefined type, making it consistent with the runtime behavior.

The default casting behavior is indeed something that we cannot easily change due to potential breakage, but I don't think your case should be considered an error either.

When using an explicit undefined default value with withDefaults, the previous return type does have a bug where it still resolves the type to boolean - this is fixed by #8602.

@matthew-dean
Copy link
Author

@yyx990803

This statement didn't make sense to me:

The interface declares foo as being optional, but it is not the type of the resolved runtime props. The resolved props type correctly excludes the undefined type, making it consistent with the runtime behavior.

I'm not sure what you mean "correctly excludes". It's not correct according to TypeScript and the runtime does support undefined as a default value, so it's perfectly consistent to include it as the default, and booleans defaulting to false was a non-sensical design choice.

@matthew-dean
Copy link
Author

matthew-dean commented Jul 18, 2023

@yyx990803 Here's an even worse, more egregious example, where Vue's type inference is way off-base.

I made a component with these props (I've reduced it to the specific problem):

interface Props {
  error?: string | true
}
defineProps<Props>()

Now, in this case, the error can only have 3 possible value types: the value true, a type of string, or undefined.

The safest thing to infer as a default value? Obviously, it's undefined, according to the types. Instead, error has a default value of false, a value that should not be possible.

In fact, if I write this:

if (props.error === false) {}

...I get the error This comparison appears to be unintentional because the types 'ReactiveVariable<string> | ReactiveVariable<true> | undefined' and 'false' have no overlap.

TypeScript itself is stating (correctly) that Vue is not handling types correctly, is abusing the type system, and is assigning values which violate those types and which could lead to runtime errors.

This is a bug. Please re-open.

@matthew-dean
Copy link
Author

At the very least, can there be changes to the Vite / Webpack plugins to fix this behavior for the defineProps macro and correctly reflect TypeScript types in the runtime (and in Volar), if the Vue team does not want to create a breaking change?

Something like:

vue({
  script: {
    accurateTypes: true
  }
})

@sxzz sxzz reopened this Jul 22, 2023
@SnosMe
Copy link

SnosMe commented Aug 24, 2023

I've found workaround is to use uppercase/class version of Boolean. Most likely your linter will complain about this. 😄But it works, and I get undefined instead of false.

@zernonia
Copy link

zernonia commented Oct 7, 2023

I've created useForwardProps as another workaround for this issue as it is quite crucial for radix-vue.

I've also pointed out some pain point and challenges here vuejs/vue#4792 (comment)

@matthew-dean
Copy link
Author

matthew-dean commented Jan 27, 2024

@SnosMe Your comment is great! I think that's actually a perfect* near perfect workaround. I paired it with this in my ESLint config:

        '@typescript-eslint/ban-types': [
          'error',
          {
            types: {
              Boolean: false
            },
            extendDefaults: true
          }
        ]
  • EDIT: I think, as far as I know, that this still causes heaps of trouble with a type like this:
interface Props {
  error?: string | true
}
defineProps<Props>()

As I note in that comment, there are only 3 possible values based on that type: undefined, true, and a string. What does Vue do? It sets it to false, a completely invalid value. (Note: this was a real type I was dealing with where validation functions could return true (valid) or a string which represented an error message. Vuetify uses this pattern.)

So, with a type like that, you're kind of outta luck unless you use withDefaults I guess? 🤔

@guaijie
Copy link
Contributor

guaijie commented Apr 11, 2024

I made a component with these props (I've reduced it to the specific problem):

请问怎么去解析这种情况:

type Props = {
  prop?: true | string
}

明确都排除了不能传 false 还是默认转换为false 吗?这个 defineProps 明显就是一个 bug ,为什么要担心破坏性更改。
有 bug 就修复啊,现今为止我身边的开发者没有不为这种默认行为感到不吃惊的 。为了等待你们的修复都转而使用 defineComponent。

image

官方明确说明了只有是 boolean 类型才会默认转换。
你这里是只要包含 boolean 类型就会转换

type Props = {
  dom?: VNodeChildAtom
}

只是因为 VNodeChildAtom 中含有 boolean 类型就给我转换为 false。我无法相信怎么会有这么反直觉的行为

这个 bug 提出这么久了,为什么还没解决了。如果是 bug 为什么要担心所谓破坏性更改了

@rcjiang
Copy link

rcjiang commented Apr 11, 2024

This issue can lead to application corruption after updating dependencies.
I only discovered this discussion after mentioning an RFC.
vuejs/rfcs#660

@TuringJest
Copy link

TuringJest commented Apr 16, 2024

FYI, the return type of defineProps does take this into account:

interface Props {
  foo?: boolean
}
const p = defineProps<Props>()
//    ^ { foo: boolean }
 p.foo
// ^ boolean

The interface declares foo as being optional, but it is not the type of the resolved runtime props. The resolved props type correctly excludes the undefined type, making it consistent with the runtime behavior.

The default casting behavior is indeed something that we cannot easily change due to potential breakage, but I don't think your case should be considered an error either.

When using an explicit undefined default value with withDefaults, the previous return type does have a bug where it still resolves the type to boolean - this is fixed by #8602.

The issue is that there is a wrong assumption at the bottom. Yes, booleans should be true or false BUT assuming you want booleans here is not correct. What is needed in the context of templates, etc. are optional booleans.

This case has been made several times and with good examples as to why this is true.
It's also the reason why this behaviour turns out to be inconsistent with TS, it is not by chance. It contradicts useful functionality of the default optional boolean value.

It's somewhat understandable how we got here but not why we have to stick with this.

@matthew-dean
Copy link
Author

matthew-dean commented Apr 16, 2024

@TuringJest

The issue is that there is a wrong assumption at the bottom. Yes, booleans should be true or false BUT assuming you want booleans here is not correct. What is needed in the context of templates, etc. are optional booleans.

100% and I'm so glad you said this. What you're defining with your TypeScript is not a boolean at all, but a union type of boolean | undefined, so Vue's mistake was to map this onto the concept of type: Boolean which is not what that interface describes.

What prop?: boolean in TypeScript actually describes is the equivalent in Vue of:

prop: {
  type: [Boolean, undefined],
  default: undefined
}

So, Vue's maintainers have made a lot of hullabaloo about this and related TS issues, saying this aligns with Vue's handling of booleans. But it doesn't because this doesn't define a boolean in TypeScript. So it has absolutely nothing to do with Vue's default value for a type of boolean, because these are not booleans. TypeScript's ?: boolean does not describe a boolean value. It's a fundamental lack of understanding of TypeScript, not a developer misunderstanding of Vue internals / defaults.

@guaijie
Copy link
Contributor

guaijie commented Apr 17, 2024

FYI, the return type of defineProps does take this into account:

interface Props {
  foo?: boolean
}
const p = defineProps<Props>()
//    ^ { foo: boolean }
 p.foo
// ^ boolean

The interface declares foo as being optional, but it is not the type of the resolved runtime props. The resolved props type correctly excludes the undefined type, making it consistent with the runtime behavior.

The default casting behavior is indeed something that we cannot easily change due to potential breakage, but I don't think your case should be considered an error either.

When using an explicit undefined default value with withDefaults, the previous return type does have a bug where it still resolves the type to boolean - this is fixed by #8602.

什么时候能把这个问题解决下,拖了很久了,这是一个严重的 bug 。特别是在基于组件二次封装时,会出现不可预料的问题,烦请看一下大家对于该问题的理解。mdn 明确说明,只有类型为 boolean 时默认才为 false ,你这里是只要包含 boolean 类型就默认为 false。并未达到对齐 mdn attrs 规范的目的。它是个 bug 就应该解决,而不是担心其破坏性更改

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ feature request New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants