Allow JS multiple declarations of ctor properties#10123
Merged
sandersn merged 9 commits intoAug 17, 2016
mastermicrosoft/TypeScript:masterfrom
allow-js-multiple-declaration-of-constructor-propertiesmicrosoft/TypeScript:allow-js-multiple-declaration-of-constructor-propertiesCopy head branch name to clipboard
Merged
Allow JS multiple declarations of ctor properties#10123sandersn merged 9 commits intomastermicrosoft/TypeScript:masterfrom allow-js-multiple-declaration-of-constructor-propertiesmicrosoft/TypeScript:allow-js-multiple-declaration-of-constructor-propertiesCopy head branch name to clipboard
sandersn merged 9 commits into
mastermicrosoft/TypeScript:masterfrom
allow-js-multiple-declaration-of-constructor-propertiesmicrosoft/TypeScript:allow-js-multiple-declaration-of-constructor-propertiesCopy head branch name to clipboard
Conversation
When a property is declared in the constructor and on the prototype of an ES6 class, the property's symbol is discarded in favour of the method's symbol. That because the usual use for this pattern is to bind an instance function: `this.m = this.m.bind(this)`. In this case the type you want really is the method's type.
Member
Author
|
@RyanCavanaugh @weswigham mind taking a look? |
| this.nothing(); | ||
| }; | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
Need a test that does this in the opposite order (with the constructor below the method declarations)
The extra `else` caused a ton of test failures!
| /* @internal */ exportSymbol?: Symbol; // Exported symbol associated with this symbol | ||
| /* @internal */ constEnumOnlyModule?: boolean; // True if module contains only const enums or other modules with only const enums | ||
| /* @internal */ isReferenced?: boolean; // True if the symbol is referenced elsewhere | ||
| /* @internal */ isDiscardable?: boolean; // True if a Javascript class property can be overwritten by a method |
Contributor
There was a problem hiding this comment.
nit: could you name the property ? (e.g isMethodOverwritable etc.)
Member
Author
There was a problem hiding this comment.
done. I ended up with isReplaceableByMethod but tell if you want me to change it.
Member
Author
|
@weswigham I just got a test timeout failure on one of the linux workers with |
Member
|
Grrr.... Being unable to predict starvation is a pain. For now, rerun the
build and I'll reduce worker count to 3.
|
Contributor
|
I like the new name 😸 |
Contributor
|
👍 |
Also simply it considerably after noticing that it's *only* called for Javascript files, so there was a lot of dead code for TS cases that never happened.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9880
When a property is declared in the constructor and on the prototype of an ES6 class, the property's symbol is discarded in favour of the method's symbol. That because the usual use for this pattern is to bind an instance function:
this.m = this.m.bind(this). In this case the type you want really is the method's type.