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

feat: runtime-support for type unions in props #12059

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

davidmatter
Copy link
Contributor

@davidmatter davidmatter commented Sep 27, 2024

One of the last missing features regarding TypeScript support in Vue is the ability to use discriminated- and distributive union types in props with runtime validation. This PR aims to address these limitations and provides a more flexible way to define props.

Fixes

Implementation

Through compiling and validating the new internal prop option union, we can skip validation for props that are not part of an active sub-union.

In the following example, a and b are in an active sub-union, c and d are not. Defining either a or b should trigger warnings for missing props for the other prop in the same sub-union.

defineProps<
  | {
      a: string
      b: number
    }
  | {
      c: string
      d: number
    }
>()

// Usage
<Comp a="foo" /> // Should trigger missing required prop warning only for `b`

Limitations

Supported patterns

This proposal currently supports the following patters:

Discriminated union example

const props = defineProps<
  | {
      tag: string
      d1: number
    }
  | {
      tag: string
      d2: string
    }
>()

// Union definition:
{
  tag: [['d1'], ['d2']],
  d1: ['tag'],
  d2: ['tag']
}

// Usage:
<Comp tag="asdf" :d1="1" /> // Should not warn regarding missing `d2`

Distributive union example

defineProps<
  | {
      a: string
      b: number
    }
  | {
      c: string
      d: number
    }
>()

// Union definition:
{
  a: ['b'],
  b: ['a'],
  c: ['d'],
  d: ['c'],
}

// Usage
<Comp a="foo" /> // Should trigger missing required prop warning only for `b`

Mentions

Shoutout to @KazariEX for helping me navigating various intricacies of the prop type system.

* fix(runtime-core): withDefaults & union types

* feat: union props

* chore: add distributive union test

* chore: improve tests
Copy link

pkg-pr-new bot commented Sep 27, 2024

Open in Stackblitz

@vue/compiler-core

pnpm add https://pkg.pr.new/@vue/compiler-core@12059

@vue/compiler-dom

pnpm add https://pkg.pr.new/@vue/compiler-dom@12059

@vue/compiler-sfc

pnpm add https://pkg.pr.new/@vue/compiler-sfc@12059

@vue/compiler-ssr

pnpm add https://pkg.pr.new/@vue/compiler-ssr@12059

@vue/reactivity

pnpm add https://pkg.pr.new/@vue/reactivity@12059

@vue/runtime-core

pnpm add https://pkg.pr.new/@vue/runtime-core@12059

@vue/runtime-dom

pnpm add https://pkg.pr.new/@vue/runtime-dom@12059

@vue/server-renderer

pnpm add https://pkg.pr.new/@vue/server-renderer@12059

@vue/shared

pnpm add https://pkg.pr.new/@vue/shared@12059

@vue/compat

pnpm add https://pkg.pr.new/@vue/compat@12059

vue

pnpm add https://pkg.pr.new/vue@12059

commit: dda57c9

Copy link

github-actions bot commented Sep 27, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 101 kB (+180 B) 38.1 kB (+70 B) 34.3 kB (+99 B)
vue.global.prod.js 160 kB (+180 B) 58 kB (+75 B) 51.6 kB (+51 B)

Usages

Name Size Gzip Brotli
createApp (CAPI only) 49.2 kB (+180 B) 19 kB (+71 B) 17.3 kB (+66 B)
createApp 55.8 kB (+180 B) 21.5 kB (+70 B) 19.7 kB (+76 B)
createSSRApp 59.9 kB (+180 B) 23.2 kB (+72 B) 21.1 kB (+103 B)
defineCustomElement 60.6 kB (+180 B) 23 kB (+67 B) 21 kB (+93 B)
overall 69.6 kB (+180 B) 26.6 kB (+68 B) 24.2 kB (+134 B)

@darkbasic
Copy link

I would like to cherry-pick both this and #12108 but I get conflicts in d09f791#diff-ddb3174016350dd4464764b4a1ba232d09ad1e75164d4a37758c759d019202e4

How would you solve it in the context of the other PR?

@davidmatter
Copy link
Contributor Author

davidmatter commented Oct 8, 2024

@edison1105 my PR contains an improvement (L331) to inferring props in combination with withDefaults. Any recommendation on how we can resolve this conflict? I need to investigate if my PR already resolves #12108. I'd be open to extract it from my PR to make it more atomic.

Edit: The test in #12108 passes when using my branch. Apparently, we've both fixed #12106, just in a different way. @edison1105 let me know which implementation you prefer. @darkbasic cherry-picking only this PR will be enough for the moment (at least according to the type tests).

@darkbasic
Copy link

Thanks. I would like to test this in my codebase, could you please share how am I supposed to pack it?
I've rebased your branch against main to be sure to include #12077
Then I've run pnpm install && pnpm run build && pnpm run release --skipGit --skipTests
I didn't find a way to pack all packages at once, so I've gone into each workspace and I've run pnpm pack, copied the tarball into my project and added the following:

  "resolutions": {
    "@vue/runtime-core": "file:./packages/vue-runtime-core-3.5.12.tgz",
    "@vue/compiler-sfc": "file:./packages/vue-compiler-sfc-3.5.12.tgz",
    "@vue/reactivity": "file:./packages/vue-reactivity-3.5.12.tgz",
    "@vue/shared": "file:./packages/vue-shared-3.5.12.tgz",
    "@vue/compiler-core": "file:./packages/vue-compiler-core-3.5.12.tgz",
    "@vue/compiler-dom": "file:./packages/vue-compiler-dom-3.5.12.tgz",
    "@vue/compiler-ssr": "file:./packages/vue-compiler-ssr-3.5.12.tgz"
  },

Unfortunately I get some errors when trying to import some types:
image
image

@davidmatter
Copy link
Contributor Author

For every commit/PR in this repo, a package is released on pkg.pr.new. You can use the published package like this:
pnpm add https://pkg.pr.new/vue@12059

In this case, 12059 points to the current PR.

@darkbasic
Copy link

But this PR doesn't include #12077 which according to the description is needed. Could you please rebase against latest main so that a new package gets released? Thanks

@darkbasic
Copy link

Unfortunately I'm still seeing runtime warnings.
This is my component with union props called InfoCard:
image
image
This is the parent component which makes use of it:
image
And this is the warning in the console:
image

I've tried removing withDefaults and I'm still getting the same issue.

These are the yarn resolutions in order to use the packages from this PR:

   "resolutions": {
-    "@vue/language-core": "file:./packages/vue-language-core-2.1.7-alpha.0.tgz"
+    "@vue/language-core": "file:./packages/vue-language-core-2.1.7-alpha.0.tgz",
+    "@vue/runtime-core": "https://pkg.pr.new/@vue/runtime-core@12059",
+    "@vue/compiler-sfc": "https://pkg.pr.new/@vue/compiler-sfc@12059",
+    "@vue/reactivity": "https://pkg.pr.new/@vue/reactivity@12059",
+    "@vue/shared": "https://pkg.pr.new/@vue/shared@12059",
+    "@vue/compiler-core": "https://pkg.pr.new/@vue/compiler-core@12059",
+    "@vue/compiler-dom": "https://pkg.pr.new/@vue/compiler-dom@12059",
+    "@vue/compiler-ssr": "https://pkg.pr.new/@vue/compiler-ssr@12059"
   },

@davidmatter
Copy link
Contributor Author

davidmatter commented Oct 9, 2024

Hey, thanks for your input. With your example provided I don't see a solution to build a proper union. Your example boils down to the following:

export interface NumberCard {
  hasChart: boolean
  chartOption: ChartOption
  type: 'number'
  value: number
}

export interface FractionCard {
  hasChart: boolean
  chartOption: ChartOption
  type: 'fraction'
  value: Fraction
}

export interface NoValueCard {
  hasChart: true
  chartOption: 'bar' | 'line'
}

export type Card = NumberCard | FractionCard | NoValueCard

In your example, it's impossible to tell if the props are missing or not because hasChart is part of all the members and hasChart = true is neither a clear distribution nor a discrimination.

@darkbasic
Copy link

Why wouldn't you be able to discriminate? hasChart is an union by itself (true | false) and if you have it as true the only viable option between NumberCard | FractionCard | NoValueCard is NoValueCard:

image

As you can see Typescript gets it right.

@darkbasic
Copy link

It looks like this needs to be rebased now that #12108 got merged.

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

Successfully merging this pull request may close these issues.

3 participants