Skip to content
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 Declarations for Classes and Interfaces #3333

Merged
merged 37 commits into from
Jul 2, 2015
Merged

Merged Declarations for Classes and Interfaces #3333

merged 37 commits into from
Jul 2, 2015

Conversation

aozgaa
Copy link
Contributor

@aozgaa aozgaa commented Jun 1, 2015

This implements the proposal in #3332.

@msftclas
Copy link

msftclas commented Jun 1, 2015

Hi @aozgaa, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Arthur Ozga). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@@ -135,6 +135,26 @@ module ts {
return node.name ? declarationNameToString(node.name) : getDeclarationName(node);
}

/* internal */
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@@ -144,7 +163,10 @@ module ts {
let symbol: Symbol;
if (name !== undefined) {
symbol = hasProperty(symbols, name) ? symbols[name] : (symbols[name] = createSymbol(0, name));
if (symbol.flags & excludes) {

// Check for declarations node cannot be merged with.
Copy link
Member

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 around node just so this reads easier? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@@ -1473,7 +1473,8 @@ module ts {
EnumMemberExcludes = Value,
FunctionExcludes = Value & ~(Function | ValueModule),
ClassExcludes = (Value | Type) & ~ValueModule,
InterfaceExcludes = Type & ~Interface,
AmbientClassExcludes = (Value | Type) & ~(ValueModule | Interface),
Copy link
Contributor

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)

Copy link
Contributor

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.

*/
function hasNonAmbientClass(symbol: Symbol): boolean {
return symbol && forEach(symbol.declarations, (element: Declaration) => {
return element.kind === SyntaxKind.ClassDeclaration && !(element.flags & NodeFlags.Ambient);
Copy link
Contributor

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
Copy link
Contributor

👍

@aozgaa
Copy link
Contributor Author

aozgaa commented Jun 17, 2015

Thoughs @mhegazy, @ahejlsberg ?

@@ -202,11 +211,11 @@ namespace ts {
symbol = hasProperty(symbolTable, name)
? symbolTable[name]
: (symbolTable[name] = createSymbol(SymbolFlags.None, name));

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@DanielRosenwasser
Copy link
Member

There are apparently some issues with the build - you probably need to fix up baselines that don't agree with the merge.

@aozgaa
Copy link
Contributor Author

aozgaa commented Jun 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
Copy link
Contributor

mhegazy commented Jun 23, 2015

👍

1 similar comment
@DanielRosenwasser
Copy link
Member

👍

aozgaa pushed a commit that referenced this pull request Jul 2, 2015
Merged Declarations for Classes and Interfaces
@aozgaa aozgaa merged commit 6775d88 into microsoft:master Jul 2, 2015
@DanielRosenwasser
Copy link
Member

This should be added to the Roadmap and What's New In TypeScript page.

@heycalmdown
Copy link

How does this work with imported module?

When I have like this application.ts

import * as amqp from 'amqplib';

// I want to merge declaration with `interface amqp.ExchangeOptions`
interface ExchangeOptions {
  'x-recent-history-length' : number;
}

interface amqp.ExchangeOptions {
  'x-recent-history-length' : number;
}

Which one should work?

@JsonFreeman
Copy link
Contributor

Neither one of those would work here. You'd need to do the following:

declare module 'amqplib' {
    interface ExchangeOptions {
       'x-recent-history-length' : number;
    }
}

@heycalmdown
Copy link

Thanks!

@masaeedu
Copy link
Contributor

@JsonFreeman Is there an equivalent for external modules?

@mhegazy
Copy link
Contributor

mhegazy commented Jan 14, 2016

@masaeedu please see #6213

@nbransby
Copy link

nbransby commented Apr 9, 2016

This doesn't appear to be possible but feels like it should:

interface Foo {
    readonly name: string
}

class Foo {
    constructor(name: string) {
        this.name = name; //error
    }    
}

@DanielRosenwasser
Copy link
Member

@nbransby thanks, I'll open an issue for you.

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.

9 participants