Skip to content
This repository has been archived by the owner on Jan 5, 2024. It is now read-only.

Double definitions not recognized #380

Closed
andevsoftware opened this issue Mar 31, 2016 · 14 comments
Closed

Double definitions not recognized #380

andevsoftware opened this issue Mar 31, 2016 · 14 comments

Comments

@andevsoftware
Copy link

I see some unexpected behaviour having several definitions that appear in multiple packages that have the same dependencies. I reach out because I'm not sure if it's me misusing it or typings needing improvement.

screen shot 2016-03-31 at 09 18 08

To test this:

clone https://github.com/redound/exp-typings.git
cd exp-typings
typings install
tsc

Then you get the above error indicating that EventEmitter and EventEmitter are incompatible. (It's the same EventEmitter).

@unional
Copy link
Member

unional commented Mar 31, 2016

It's fixed in latest release

@unional unional closed this as completed Mar 31, 2016
@unional unional reopened this Mar 31, 2016
@unional unional removed the duplicate label Mar 31, 2016
@andevsoftware
Copy link
Author

@unional I think it's a separate issue, I've updated to that update before I issued this.

@unional
Copy link
Member

unional commented Mar 31, 2016

Yup. Just realized that.

@andevsoftware
Copy link
Author

@unional Any idea what the problem might is?

@unional
Copy link
Member

unional commented Mar 31, 2016

Need the master to handle this.
I can see what's the issue, but...

As said in the error, the protected field is what caused the incompatibility. (private fields too).

If I remove protected _eventCallbacks: any; from EventEmitter and private _params from Event<T>, it build successfully.

Private properties shouldn't be included in declaration, for sure.

I'm not sure about protected properties. Likely this involves TypeScript team.

@blakeembrey thoughts?

@blakeembrey
Copy link
Member

I'll look through existing TypeScript issues to understand the reasoning here from TypeScript's team, but it's pretty simple to verify in a standalone test:

export class Test {
  private method () {}
}

export class Test2 {
  private method () {}
}

export function compare (t: Test) {}

compare(new Test2())
index.ts(11,9): error TS2345: Argument of type 'Test2' is not assignable to parameter of type 'Test'.
  Types have separate declarations of a private property 'method'.

@blakeembrey
Copy link
Member

Seems like microsoft/TypeScript#6496 and microsoft/TypeScript#6365 are relevant here. I can't tell the real reasoning right now why private and protected changes it from a structural to a nominal type check, it seems to me they should all be structural and private or protected shouldn't even be considered in assignability because they can't be accessed outside of the class.

@andevsoftware
Copy link
Author

@blakeembrey Thanks clearing that up. What would you recommend for now so that the Item satisfies the Model? I've made the property protected which causes the same error. The only route is to make it public i guess?

@blakeembrey
Copy link
Member

From the spec: https://github.com/Microsoft/TypeScript/blob/master/doc/spec.md#3112-type-and-member-identity. You can create an interface inline that describes what the value should conform to instead of using the class type. E.g. function accepted (e: { on (type: string, fn: (...args: any[]) => any)): any }). That'll make the check properly structural instead of based on identity until such a time that Typings can optimize the compilation output to avoid duplicates like this (or TypeScript 2.0).

@blakeembrey
Copy link
Member

I'm going to open an issue to see if I can understand more, at the very least it'll be closed as a duplicate but it'll alert them that it's maybe an issue.

@blakeembrey
Copy link
Member

Ah, I see you already created microsoft/TypeScript#7743 too. I guess we'll see what gets said anyway.

@andevsoftware
Copy link
Author

@blakeembrey Yes, we'll see.

@blakeembrey
Copy link
Member

Ok, looks like making an abstract interface may be the best way for now until Typings/TypeScript can handle de-duping better.

@blakeembrey
Copy link
Member

Closing as answered for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants