- Notifications
You must be signed in to change notification settings - Fork13.2k
Merged Declarations for Classes and Interfaces#3333
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This reverts commit813d227.
msftclas commentedJun 1, 2015
Hi@aozgaa, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
TTYL, MSBOT; |
src/compiler/binder.ts Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This isn't exported, so you don't need this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed.
src/compiler/binder.ts Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: Can you put single quotes aroundnode just so this reads easier? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed.
src/compiler/types.ts Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you do the other change I suggested (https://github.com/Microsoft/TypeScript/pull/3333/files#r31552915), then this goes away, and ClassExcludes becomes(Value | Type) & ~(ValueModule | Interface)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why not just letting classes and interfaces merge, and reporting an error later on in CheckClassDeclaration for instance if the class is not ambient? just like we do with merging order for module/class and module/class being in different files.
src/compiler/binder.ts Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should not this be inAmbientContext instead?
JsonFreeman commentedJun 4, 2015
👍 |
aozgaa commentedJun 17, 2015
Thoughs@mhegazy,@ahejlsberg ? |
src/compiler/binder.ts Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove the leading space if you get the chance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed.
DanielRosenwasser commentedJun 18, 2015
There are apparently some issues with the build - you probably need to fix up baselines that don't agree with the merge. |
aozgaa commentedJun 18, 2015
@DanielRosenwasser The error codes needed to be changed as part of merging the upstream fixes and this broke some of the baselines. No other errors occurred. They've been added now :). |
mhegazy commentedJun 23, 2015
👍 |
1 similar comment
DanielRosenwasser commentedJun 23, 2015
👍 |
Merged Declarations for Classes and Interfaces
DanielRosenwasser commentedAug 13, 2015
This should be added to the |
heycalmdown commentedOct 4, 2015
How does this work with imported module? When I have like this application.ts import*asamqpfrom'amqplib';// I want to merge declaration with `interface amqp.ExchangeOptions`interfaceExchangeOptions{'x-recent-history-length' :number;}interfaceamqp.ExchangeOptions{'x-recent-history-length' :number;} Which one should work? |
JsonFreeman commentedOct 5, 2015
Neither one of those would work here. You'd need to do the following: declare module'amqplib'{interfaceExchangeOptions{'x-recent-history-length' :number;}} |
heycalmdown commentedOct 7, 2015
Thanks! |
masaeedu commentedJan 14, 2016
@JsonFreeman Is there an equivalent for external modules? |
mhegazy commentedJan 14, 2016
nbransby commentedApr 9, 2016
This doesn't appear to be possible but feels like it should: interfaceFoo{readonlyname:string}classFoo{constructor(name:string){this.name=name;//error}} |
DanielRosenwasser commentedApr 9, 2016
@nbransby thanks, I'll open an issue for you. |
This implements the proposal in#3332.