Allow JS multiple declarations of ctor properties #10123
Merged
Conversation
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
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.
|
@RyanCavanaugh @weswigham mind taking a look? |
| @@ -7,4 +7,19 @@ function C() { | |||
| } | |||
| C.prototype.m = function() { | |||
| this.nothing(); | |||
| }; | |||
| } | |||
|
|
|||
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!
| @@ -2137,6 +2137,7 @@ namespace ts { | |||
| /* @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 | |||
There was a problem hiding this comment.
nit: could you name the property ? (e.g isMethodOverwritable etc.)
There was a problem hiding this comment.
done. I ended up with isReplaceableByMethod but tell if you want me to change it.
|
@weswigham I just got a test timeout failure on one of the linux workers with |
|
Grrr.... Being unable to predict starvation is a pain. For now, rerun the
build and I'll reduce worker count to 3.
|
|
I like the new name 😸 |
|
👍 |
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.
sandersn
deleted the
allow-js-multiple-declaration-of-constructor-properties
branch
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.