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/2430 sync modifier prop type checking #2431

Conversation

andrewisaburden
Copy link
Contributor

Fixes #2430

andrewisaburden and others added 5 commits September 23, 2020 15:55
v-bind modifiers should always be culled from the node name,
because we cannot bind one Vue prop twice (duplicates for
v-bind are an error), and so that we aren't trying to find a
prop with name "item.sync" rather than "item".
@@ -251,7 +251,7 @@ export function getTemplateTransformFunctions(ts: T_TypeScript, childComponentNa
// Attribute name is specified
// e.g. v-bind:value="foo"
const fullName =
dir.key.modifiers.length === 0
dir.key.modifiers.length === 0 || !isVOn(dir)
Copy link
Member

Choose a reason for hiding this comment

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

I think && isVBind(dir) more intuitive.

Copy link
Contributor Author

@andrewisaburden andrewisaburden Nov 8, 2020

Choose a reason for hiding this comment

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

@yoyo930021 I agree, and initially that's what I did. However, for reasons that are not clear to me, using isVBind with the typeguard type predicate of : node is AST.VDirective results in line 256 throwing this TypeScript error: Property 'key' does not exist on type 'never'. for the section ...dir.key.modifiers.map.

This doesn't make sense to me, because dir is already of type AST.VDirective in the function parameters. I couldn't figure out a solution, if you know one please let me know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After reading a little more, typeguards that are type predicates only do one thing: confirm the type is guarded.

isVBind actually should be split into two methods, one for isVDirective and one for isVBind. isVDirective should be a type guard, e.g. function isVDirective(node: AST.VAttribute | AST.VDirective): node is AST.VDirective {
and isVBind should not be a type guard: function isVBind(node: AST.VAttribute | AST.VDirective): boolean {

Andrew Burden and others added 2 commits November 9, 2020 09:41
Type guard functions that use type predicates only do one thing:
They confirm that the type of the parameter is as specified in the
predicate.

However, functions like `isVBind` were also checking string
properties to check the content of the object structure, which
made them do more than just confirm that the type is
VDirective. This meant that the functions were being used
as conditionals for more than just their type guarding, giving
unexpected results.

The solution is to create a function, `isVDirective`, which is the
type guard for `node is AST.VDirective`, and change the returns
of the other `isVX` functions to be `boolean`.
@yoyo930021 yoyo930021 requested a review from ktsn November 9, 2020 02:28
@yoyo930021
Copy link
Member

@ktsn If you have time, you can take a look.
I think I'll merge first. If you have any questions, let me know.

@yoyo930021 yoyo930021 merged commit 95874e4 into vuejs:master Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v-bind with .sync modifier and Vue template interpolation/type checking gives TypeScript error
2 participants