Skip to content

Discriminate jsx contextual types same as object contextual types #27408

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

Merged

Conversation

weswigham
Copy link
Member

Fixes #26004

@RyanCavanaugh
Copy link
Member

Might fix #27211 as well?

}
return contextualType;
}

/**
* Woah! Do you really want to use this function?
Copy link
Member

Choose a reason for hiding this comment

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

Pretty sure it's spelled "Whoa" @DanielRosenwasser

if (!(contextualType && contextualType.flags & TypeFlags.Union && isObjectLiteralExpression(node))) {
return contextualType;
}
function discriminateContextualTypeByObjectMembers(node: ObjectLiteralExpression, contextualType: UnionType) {
// Keep the below up-to-date with the work done within `isRelatedTo` by `findMatchingDiscriminantType`
Copy link
Member

Choose a reason for hiding this comment

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

Since this applies to two functions now, enumerate the functions.

Actually, if you can extract the core of this algorithm and call it, that would be better. I'm not sure what you can do with that hilarious use of break propLoop in there. Maybe that could become a return?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

I like this fix, but can you see whether removal of some of the duplication is possible? Otherwise we'll now have 3 copies of this one weird algorithm.

@sandersn
Copy link
Member

sandersn commented Oct 5, 2018

Looks like there is some assertion lint.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Couple of minor suggestions.

}
}
if (possiblyDiscriminant) {
const result = getDiscriminationResultForProperty(contextualType, discriminatingType!, prop.symbol.escapedName, match, isTypeAssignableTo);
Copy link
Member

Choose a reason for hiding this comment

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

You could get rid of the nullable assertion if you dropped possiblyDiscriminant and checked discriminatingType above. The no-initialiser case would need to change to discriminatingType = !!isDiscr... && trueType.

Not sure if it's worth the change. Probably?

@weswigham
Copy link
Member Author

@sandersn I did better; you probably wanna look again (again), since AFAIK none of your last set of comments apply anymore.

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

This version is ok, although to be honest I liked the previous one a little better. This one makes me wonder if passing tuples of closures is a bit slow, whether we should measure it, etc.

@weswigham weswigham merged commit 07dbd8b into microsoft:master Oct 5, 2018
@weswigham weswigham deleted the react-union-sfc-contextualtype-test branch October 5, 2018 22:11
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.

3 participants