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(runtime-core): support typed safe vnode ref on the template #5387

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

Conversation

pikax
Copy link
Member

@pikax pikax commented Feb 9, 2022

Support typesafe :ref property when used on the template.

This PR adds support when you use <video :ref="(e)=> e?.playsInline"> or type safe with used with Ref

const videoEl = ref<HTMLVideoElement | null>(null)

// works 
;<video ref={videoEl}/>

// @ts-expect-error HTMLAnchorElement not assigned to HTMLVideoElement
;<a ref={videoEl}/>

This also brings type safe to custom components and functional components, altho functional components will be converted to ComponentPublicInstance<Props> and type safe in that scenario will be a bit trickier for the user.

const FuncComp = (props: { foo: string}) => h('div', props.foo);

/* Is not easy to retrieve the correct type when used like a plain function, recommended using ComponentPublicInstance */  
const CompInstance = ref<ComponentPublicInstance<{ foo: string}> | null  >();

;<FuncComp :ref={CompInstance}/>

// or 

// type is correct when using function
;<FuncComp :ref={x=> x?.$props.foo}/>

DefineComponent works out of the box with the expected type being provided, this might cause some problems on some code bases, depending on how the users are declaring their refs, but in most cases it should work

const Comp = defineComponent({
  props: {
    test: String
  }
})


declare let videoElRef: Ref<HTMLVideoElement | null>

// Recommended way to get the Component type is to get the InstanceType

declare let myComp: InstanceType<typeof Comp> | null
declare let myCompRef: Ref<InstanceType<typeof Comp> | null>
declare let anyComp: ComponentPublicInstance | null


expectType<JSX.Element>(<Comp ref={e => (myComp = e)} />)
expectType<JSX.Element>(<Comp ref={myCompRef} />)
expectType<JSX.Element>(<Comp ref={e => (anyComp = e)} />)

Drops the typescript support for TSX ref={'str'}

Closes #3455

@pikax pikax added scope: types 🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. labels Feb 9, 2022
@netlify
Copy link

netlify bot commented Feb 9, 2022

✔️ Deploy Preview for vue-next-template-explorer ready!

🔨 Explore the source changes: 119557a

🔍 Inspect the deploy log: https://app.netlify.com/sites/vue-next-template-explorer/deploys/620398f476208d00075a6e80

😎 Browse the preview: https://deploy-preview-5387--vue-next-template-explorer.netlify.app

@netlify
Copy link

netlify bot commented Feb 9, 2022

✔️ Deploy Preview for vuejs-coverage ready!

🔨 Explore the source changes: 119557a

🔍 Inspect the deploy log: https://app.netlify.com/sites/vuejs-coverage/deploys/620398f4908a2c000802818b

😎 Browse the preview: https://deploy-preview-5387--vuejs-coverage.netlify.app

@netlify
Copy link

netlify bot commented Feb 9, 2022

✔️ Deploy Preview for vue-sfc-playground ready!

🔨 Explore the source changes: 119557a

🔍 Inspect the deploy log: https://app.netlify.com/sites/vue-sfc-playground/deploys/620398f447206d000810f657

😎 Browse the preview: https://deploy-preview-5387--vue-sfc-playground.netlify.app/

@LinusBorg
Copy link
Member

Hey @pikax,

This is looking good!

Is there any risk this might cause type errors in current usage in specific scenarios? In that case we might postpone this to 3.3.

If it's fully backwardds-compatible, we an merge it in a patch. I'd assume it is, since people were forced to typecast previously anyways, right?

@pikax
Copy link
Member Author

pikax commented Feb 19, 2022

Is there any risk this might cause type errors in current usage in specific scenarios? In that case we might postpone this to 3.3.

The scenarios I can think of are if ppl are declaring wrong refs:

const comp = ref<ComponentPublicInstance | undefined>() // the correct type should have `null` too!
const div = ref<HTMLElement>() // this will fail without `| null` 

If it's fully backwardds-compatible, we an merge it in a patch. I'd assume it is, since people were forced to typecast previously anyways, right?

I think most of the cases ppl use the string version of ref, if you using the typescript version I would expect if errors show up, to be actual errors (aside from | null)

pikax added 2 commits October 6, 2023 17:14
# Conflicts:
#	packages/dts-test/componentRef.test-d.tsx
#	packages/runtime-core/src/vnode.ts
#	packages/runtime-dom/src/jsx.ts
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 85.9 kB 32.7 kB 29.5 kB
vue.global.prod.js 132 kB 49.4 kB 44.3 kB

Usages

Name Size Gzip Brotli
createApp 47.9 kB 18.9 kB 17.2 kB
createSSRApp 50.7 kB 20 kB 18.2 kB
defineCustomElement 50.3 kB 19.7 kB 18 kB
overall 61.3 kB 23.7 kB 21.6 kB

@pikax pikax requested review from haoqunjiang and sxzz October 6, 2023 16:51
@edison1105 edison1105 added the ready for review This PR requires more reviews label Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍰 p2-nice-to-have Priority 2: this is not breaking anything but nice to have it addressed. ready for review This PR requires more reviews scope: types
Projects
Status: Needs Review
Status: No status
Development

Successfully merging this pull request may close these issues.

Allow constraining ref attribute to not allow string when using tsx.
3 participants