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

Typings: Improve $slots and $scopedSlots type to prevent unchecked access to undefined #8946

Merged
merged 3 commits into from Dec 1, 2018

Conversation

@thenickname
Copy link
Contributor

@thenickname thenickname commented Oct 14, 2018

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

When working directly with render functions in TypeScript / tsx, currently the following code is unsafe and unchecked by the ts compiler:

this.$sopedSlots.default() // default can be undefined

The change in this PR makes TypeScript complain about the unsafe access accordingly and the code has to be expressed in a way that prevents calling an undefined function, e.g.:

( this.$scopedSlots.default || ( () => 'default value' ) )( props )
@ktsn
Copy link
Member

@ktsn ktsn commented Oct 15, 2018

Thank you for making this PR. However including undefined in the scoped slots value is actually inaccurate.

In the TypeScript, index signature is not guarantees whether the referenced key is exists. This is similar that an array type can be return undefined if accessing index of out of length.

const obj: Record<string, number> = { foo: 1, bar: 2 }
obj.baz // -> undefined

const arr: number[] = [1, 2, 3]
arr[5] // -> undefined

Some case that Including undefined is appropriate would be the case that key is exists but the value is undefined such as:

const obj: Record<string, number | undefined> = { foo: 1, bar: undefined }

Object.values() type would help you to understand these differences:

const a: Record<string, number | undefined> = { a: 1 }

// will be `(number | undefined)[]` even though `undefined` will be never appeared
const b = Object.values(a) 

@thenickname
Copy link
Contributor Author

@thenickname thenickname commented Oct 15, 2018

Hey, thank you for the quick reaction.

In the TypeScript, index signature is not guarantees whether the referenced key is exists.

Actually, I believe that it effectively does guarantee the following: It tells the TypeScript compiler that every possible key is permitted and (thats the important part) that all the values of the given object are guaranteed to have actual content of the type ScopedSlot.

Let me give you an example to hopefully better illustrate what is happening with the current type definition:

Let's first look at the current type definition for $scopedSlots:

interface Vue {
    readonly $scopedSlots: { [key: string]: ScopedSlot };
}

Now, in a vue component we try to call the default slot function:

this.$scopedSlots.default( props );

TypeScript won't complain about this code. But at runtime, default slot could be empty. The code above in that case will lead to a runtime error, because default is not a function and hence not callable.

Now if we tweak the type definition like that:

interface Vue {
    readonly $scopedSlots: { [key: string]: ScopedSlot | undefined };
}

This is what happens:

bildschirmfoto 2018-10-16 um 00 41 20

It says cannot invoke an object which is possibly undefined. And that is a good thing. It forces you to do a safety check before calling the function. For example like that:

<div>
    {this.$scopedSlots.default ?
        this.$scopedSlots.default( props ) :
        'Fallback content'
    }
</div>

So, to sum it up:

Right now the TypeScript compiler won't complain about slot access because of the current type defintition for $scopedSlots. Because the content that is referenced by the key is defined to always be a ScopedSlot, TypeScript will happily allow a function call even if no slot function is present. It does not account for the fact that at runtime a slot is not necessarily defined.

I hope this explanation better illustrates why I believe this PR is an improvement of the current typing.

types/vue.d.ts Outdated
@@ -28,7 +28,7 @@ export interface Vue {
readonly $children: Vue[];
readonly $refs: { [key: string]: Vue | Element | Vue[] | Element[] };
readonly $slots: { [key: string]: VNode[] };
Copy link
Contributor

@KaelWD KaelWD Oct 20, 2018

Non-scoped slots can be undefined too.

Copy link
Contributor Author

@thenickname thenickname Oct 22, 2018

thanks for pointing that out! Will add that to this PR..

@ktsn
Copy link
Member

@ktsn ktsn commented Oct 22, 2018

I see your point. As scoped slot and slot optional, it may be better to have undefined value 🙂

I was thinking scoped slot and slot always there if the object key is exists but it was incorrect as they can be blank. Not sure how actual implementation is but it sounds good to have undefined in that case.

declare $slots option as potentially undefined to enable stricter TS checks
@thenickname thenickname changed the title Typings: Improve $scopedSlots type to prevent unchecked access to undefined Typings: Improve $slots and $scopedSlots type to prevent unchecked access to undefined Oct 22, 2018
ktsn
ktsn approved these changes Oct 23, 2018
@yyx990803 yyx990803 merged commit 7644380 into vuejs:dev Dec 1, 2018
5 checks passed
KaelWD added a commit to KaelWD/vue that referenced this issue Dec 3, 2018
@KaelWD KaelWD mentioned this pull request Dec 3, 2018
12 tasks
@KaelWD
Copy link
Contributor

@KaelWD KaelWD commented Dec 3, 2018

You missed the one in VNodeData, I've fixed it with #9131

@thenickname
Copy link
Contributor Author

@thenickname thenickname commented Dec 3, 2018

@KaelWD thanks

f2009 pushed a commit to f2009/vue that referenced this issue Jan 25, 2019
…cess to undefined (vuejs#8946)

* fix(types): Declare $scopedSlots as potentially undefined to enable stricter TS checks

* fix(types): Fix tests

* fix(types): declare $slots option as potentially undefined

declare $slots option as potentially undefined to enable stricter TS checks
f2009 pushed a commit to f2009/vue that referenced this issue Jan 25, 2019
erdong-fe pushed a commit to erdong-fe/vue that referenced this issue May 2, 2020
…cess to undefined (vuejs#8946)

* fix(types): Declare $scopedSlots as potentially undefined to enable stricter TS checks

* fix(types): Fix tests

* fix(types): declare $slots option as potentially undefined

declare $slots option as potentially undefined to enable stricter TS checks
erdong-fe pushed a commit to erdong-fe/vue that referenced this issue May 2, 2020
aJean added a commit to aJean/vue that referenced this issue Aug 19, 2020
…cess to undefined (vuejs#8946)

* fix(types): Declare $scopedSlots as potentially undefined to enable stricter TS checks

* fix(types): Fix tests

* fix(types): declare $slots option as potentially undefined

declare $slots option as potentially undefined to enable stricter TS checks
aJean added a commit to aJean/vue that referenced this issue Aug 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants