Skip to content

Fix prototype property type lookup #33034

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
merged 2 commits into from
Aug 22, 2019
Merged

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Aug 22, 2019

This removes over half of the weird code for supporting JS in getTypeReferenceType. Lookup of commonjs aliases is still there and still weird, but I improved the documentation a little.

Fixes #33013

I knew I missed some code in the constructor-functions-as-classes PR.
This simplifies the type reference resolution code as well.
@sandersn
Copy link
Member Author

@andrewbranch @PranavSenthilnathan you might be interested in this. Pranav, I remember you made the last edit to the documentation of getTypeFromJSAlias. It's used only for aliases now, so much of the previous text didn't apply anymore.

@andrewbranch
Copy link
Member

@orta might be interested in looking at it too after he gets back, since he fixed a couple expando-related bugs recently.

@sandersn sandersn requested a review from orta August 22, 2019 17:16
const valueType = getTypeOfSymbol(symbol);
const typeType =
valueType.symbol &&
valueType.symbol !== symbol && // Make sure this is a commonjs export by checking that symbol -> type -> symbol doesn't roundtrip.
Copy link
Member

Choose a reason for hiding this comment

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

We do have tests that still fall thru and use this function, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, about 6. None of them are classes anymore, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Believe me, I tried deleting it to make sure. =P

Copy link
Member Author

@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.

Some explanations of what changed.

const type = getTypeReferenceTypeWorker(node, symbol, typeArguments);
if (type) {
return type;
if (symbol.flags & (SymbolFlags.Class | SymbolFlags.Interface)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

these two cases are all that's left of getTypeReferenceTypeWorker

@@ -9195,10 +9211,13 @@ namespace ts {
if (symbol === unknownSymbol) {
return errorType;
}
symbol = getExpandoSymbol(symbol) || symbol;
Copy link
Member Author

Choose a reason for hiding this comment

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

getExpandoSymbol takes care of the JS special cases that used to be in getTypeReferenceTypeWorker; then you just treat them like normal symbols afterward.

if (resolved.callSignatures.length === 1) {
return getReturnTypeOfSignature(resolved.callSignatures[0]);
}
function getTypeFromJSAlias(node: NodeWithTypeArguments, symbol: Symbol): Type | undefined {
Copy link
Member Author

Choose a reason for hiding this comment

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

new name for getJSDocTypeReference, reflecting its reduced responsibilities.

@sandersn sandersn merged commit 4bddf55 into master Aug 22, 2019
@sandersn sandersn deleted the fix-prototype-property-type-lookup branch August 22, 2019 19:51
timsuchanek pushed a commit to timsuchanek/TypeScript that referenced this pull request Sep 11, 2019
* Fix constructor function type reference lookup

I knew I missed some code in the constructor-functions-as-classes PR.
This simplifies the type reference resolution code as well.

* Simplify and document js alias type resolution
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.

JS: Prototype property assignment prevents property lookup from type reference
3 participants