Skip to content

Print js-constructor function type names#23089

Merged
sandersn merged 3 commits into
masterfrom
js/improve-constructor-function-names
Apr 4, 2018
Merged

Print js-constructor function type names#23089
sandersn merged 3 commits into
masterfrom
js/improve-constructor-function-names

Conversation

@sandersn
Copy link
Copy Markdown
Member

@sandersn sandersn commented Apr 2, 2018

Instead of printing them as a type literal, which is scary.

Also, print the names of otherwise-anonymous functions and classes as the name of the declaration that they are assigned to:

var o = {
  C: class { }
}
var oc = new o.C()

In both Typescript and Javascript, we currently print "(Anonymous class)" for the type of oc. After this change, we print it as C. This isn't perfect (the accessible symbol chain isn't there), but it's better than what we have.

Instead of printing them as a type literal, which is scary.
@sandersn sandersn requested a review from weswigham April 2, 2018 21:01
exports.n.K = function () {
>exports.n.K = function () { this.x = 10;} : () => void
>exports.n.K : () => void
>exports.n.K = function () { this.x = 10;} : typeof (Anonymous function)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rather than (Anonymous function), can we wire this up so it has name K?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, I'm working on it. I'm trying to understand the machinery that decides between K, n.K and exports.n.K.

That otherwise have no name. This helps quick info for javascript a
*lot*. Typescript mainly benefits when printing the type of class
expressions.
Comment thread src/compiler/utilities.ts
else if (isPropertyAssignment(node.parent)) {
return node.parent.name;
}
else if (isBinaryExpression(node.parent) && node === node.parent.right) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No love for binding pattern initializers?:

const {
a = function() {},
b = class {}
} = ({} as any);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done! (But, no, I don't like binding pattern initializers)

@sandersn
Copy link
Copy Markdown
Member Author

sandersn commented Apr 2, 2018

Looks like I forgot to update some fourslash tests. I'll do that and then add support for binding pattern initializers.

Also fix some fourslash baselines
Copy link
Copy Markdown
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

The changes to getNameOfDeclaration may impact services for the stuff that now has a name (likely in a good way, but I'm not sure). I'd keep an eye on it.

@sandersn sandersn merged commit dca3a94 into master Apr 4, 2018
@sandersn sandersn deleted the js/improve-constructor-function-names branch April 4, 2018 22:43
@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants