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

Interface merge of import/local should error when the import is re-exported with export * #9532

Closed
unional opened this issue Jul 6, 2016 · 29 comments
Assignees
Labels
Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed

Comments

@unional
Copy link
Contributor

unional commented Jul 6, 2016

TypeScript Version: (2.0.0-dev.20160705)

Code

// foo.ts
declare module 'A' {
  export interface Foo<S> {
    (): void;
  }
}
declare module '~B' {
  import { Foo as Roo } from 'A';
  interface Foo extends Roo<any> {
  }
}
declare module 'B' {
export * from '~B';
}

// boo.ts
declare module 'B' {
  interface Foo {
     (message: string): any;
  }
}

export const SOME = 1;

// go.ts
import { Foo } from 'B';

let x: Foo;
x(); // <-- only have (message: string): any

Expected behavior:
x(); have overloads of (message: string): any and (): void

Actual behavior:
x(); only have (message: string): any

If I change boo.ts to:

// boo.ts
import { Foo as Soo } from 'A';
declare module 'B' {
  interface Foo extends Soo<any> { // <-- i.e. matching original extends
     (message: string): any;
  }
}

export const SOME = 1;

Then it works correctly.

Also, if I reference ~B instead of B (i.e. without re-export), it works correctly too:

// boo.ts
declare module '~B' {
  interface Foo {
     (message: string): any;
  }
}

export const SOME = 1;

// go.ts
import { Foo } from '~B';

let x: Foo;
x()
@unional
Copy link
Contributor Author

unional commented Jul 6, 2016

cc @blakeembrey to see if it rings any bell on similar issues.

@mhegazy mhegazy added the Bug A bug in TypeScript label Jul 6, 2016
@mhegazy mhegazy added this to the TypeScript 2.0.1 milestone Jul 6, 2016
@unional
Copy link
Contributor Author

unional commented Jul 6, 2016

Expanding this to cover one more issue:

// foo.ts
declare module 'A' {
  export interface Foo<S> {
    (): void;
  }

  export class SomeClass<S> {
    foo: Foo<S>;
  }
}
declare module '~B' {
  import { Foo as Roo, SomeClass } from 'A';
  export { SomeClass }
  interface Foo extends Roo<any> {
  }
}
declare module 'B' {
export * from '~B';
}

// boo.ts
import { Foo as Soo } from 'A';

declare module 'B' {
  interface Foo extends Soo<any> { // <-- i.e. matching original extends
    (message: string): any;
  }
}

export const SOME = 1;

// go.ts
import { Foo, SomeClass } from 'B';

let x: Foo;
x(); // <-- has overloads

let y: SomeClass<any>;
y.foo(); // <-- no overloads

@unional
Copy link
Contributor Author

unional commented Jul 6, 2016

This issue is more common than I thought.

It would happen if I wrote a module distributed through npm this way:

// index.ts
export * form './a';
export * from './b';

// a.ts
...

// b.ts
...

Which is a pretty common pattern.

@paleo
Copy link

paleo commented Jul 7, 2016

Maybe related: #9127

@sandersn
Copy link
Member

sandersn commented Jul 7, 2016

This is basically a duplicate of #8140. You can't augment things that are imported from a module, even types. But the bad error you found is different than #8140. export * from "~B" doesn't report the error, but export { Foo } from "~B" does. The compiler should give the same error for export * that it does for export { Foo }.

I'll give more detail in a subsequent comment. I'll include a modified repro and an explanation of why this fails to do what you want.

@sandersn sandersn changed the title re-export + augmentation does not work correctly unless extends matches Interface merge of alias/local should error when the alias is re-exported with export * Jul 7, 2016
@unional
Copy link
Contributor Author

unional commented Jul 7, 2016

You can't augment things that are imported from a module, even types

Not sure if I understand. boo.ts is not importing module B, it just augment module B.

@sandersn
Copy link
Member

sandersn commented Jul 7, 2016

Here's the explanation I promised. I assume that you are trying to augment the Foo declared in ~B based on what you expect to happen. The declare module 'B' block in boo.ts is supposed to do this. Unfortunately, in foo.ts, B re-exports Foo from ~B, so you can't -- once you import something, it will never merge:

declare module 'B' {
  export * from '~B'; // import and re-export `Foo`
}

TypeScript should error here. And it does -- if you explicitly say export { Foo } from '~B'. It incorrectly fails to error with export * from '~B'.

In boo.ts, you could declare module '~B' and augment ~B instead of B instead. This works as long as all your modules are originally declared as ambient module declarations (that is, declare module 'X' blocks in a file consisting only of declare module 'X' blocks).

@sandersn
Copy link
Member

sandersn commented Jul 7, 2016

I forgot to mention earlier that TypeScript skips the re-exported Foo from B that it should have errored on. That's why the base members are missing that you expect to see.

Here's the modified repro that uses properties:

// foo.ts
declare module 'A' {
  export interface Foo {
    foo: number; // diff 1: use property instead of call signature
  }
}
declare module '~B' {
  import { Foo as Roo } from 'A';
  interface Foo extends Roo {
      baz: boolean; // diff 2: add property
  }
}
declare module 'B' {
export * from '~B';
}

// boo.ts
declare module 'B' {
  interface Foo {
     bar: string; // diff 3: use property instead of call sig
  }
}
export const SOME = 1;

// go.ts
import { Foo } from 'B';

let x: Foo = {  // diff 4: initialise to object literal
    foo: 1,  // error: 'foo' does not exist in type 'Foo'
    bar: 'maybe', 
    baz: false, // error: 'baz' does not exist in type 'Foo'
};

The simplest fix is a one-liner. In boo.ts, augment '~B' instead. This allows Foo to merge before being exported by B:

// boo.ts
declare module '~B' {
  interface Foo {
    bar: string;
  }
}

I tried your fix to this modified repro but it still says that 'baz' does not exist in type 'Foo':

// boo.ts
import { Foo as Soo } from 'A';
declare module 'B' {
  interface Foo extends Soo {
     bar: string;
  }
}
export const SOME = 1;

The reason that this doesn't work is that the re-export is still skipped so baz is still missing (and typescript still fails to error). But since Foo now explicitly extends Soo, the compiler at least knows that foo is a member of Foo.

@unional
Copy link
Contributor Author

unional commented Jul 7, 2016

Does that means module written like this:

// index.ts
export * form './a';
export * from './b';

// a.ts
...

// b.ts
...

cannot be augmented?

@unional
Copy link
Contributor Author

unional commented Jul 7, 2016

Also, this could cause issue on typings if the re-export can't be merged.

@sandersn sandersn modified the milestones: TypeScript 2.1, TypeScript 2.0.1 Jul 7, 2016
@unional
Copy link
Contributor Author

unional commented Jul 7, 2016

When you say "Interface merge of alias/local should error when the alias is re-exported with export *", do you mean it is the alias Roo in import { Foo as Roo } from 'A'; causing the issue here?

@sandersn
Copy link
Member

sandersn commented Jul 7, 2016

Which module do you want to augment? index.ts? That's correct. You are not allowed to modify things that are imported from modules. Looking at existing libraries on DefinitelyTyped, it looks like people either use /// <reference ... or else put the whole core of their library into index.d.ts.

@sandersn
Copy link
Member

sandersn commented Jul 7, 2016

No, it is the export * from '~B'. If you rewrite that line as export { Foo } from '~B', you'll see the error "Export declaration conflicts with exported declaration of 'Foo': interface Foo, import Foo".

I think this could indeed be an issue for typings. I'll bring it to the attention of @RyanCavanaugh.

@sandersn
Copy link
Member

sandersn commented Jul 7, 2016

The term "alias" is misleading. Internally, the compiler refers to any imported symbol as an alias, whether it actually uses the as alias syntax or not.

@sandersn sandersn changed the title Interface merge of alias/local should error when the alias is re-exported with export * Interface merge of import/local should error when the import is re-exported with export * Jul 7, 2016
@unional
Copy link
Contributor Author

unional commented Jul 7, 2016

use /// <reference ... or else put the whole core of their library into index.d.ts

Do you mean index.ts? This limitation would definitely surprise many people.

@unional
Copy link
Contributor Author

unional commented Jul 7, 2016

see the error "Export declaration conflicts with exported declaration of 'Foo': interface Foo, import Foo"

In foo.ts, I do not see that error using 7/5 build
image

Same when using your example
image

@sandersn
Copy link
Member

sandersn commented Jul 7, 2016

a .d.ts is just a TypeScript file that contains only declarations -- but projects like Angular put a lot of declarations in their top-level declaration file, not just re-exports. I think you are right that this limitation will surprise people, however.

What tool fails to produce the error? I see it in VS Code and running tsc from the command line. I pulled new bits from github this morning.

@unional
Copy link
Contributor Author

unional commented Jul 7, 2016

What tool fails to produce the error?

That's strange. I am using VSCode. It doesn't work for both past and today's release. With 7/5 TS nightly.

@unional
Copy link
Contributor Author

unional commented Jul 7, 2016

Is this a technical limitation or there are use case for this limitation?
This will break all augmentation for typings I believe. cc @blakeembrey

@blakeembrey
Copy link
Contributor

Thanks @unional, sounds like it. I had assumed this was fixed, but I guess I reviewed it incorrectly. For reference, there's a whole chain of issues starting from typings/typings#525 (comment). Also, being unable to augment any export * modules is a huge gotcha. It's fairly common to write TypeScript that exports from multiple files as the main file (E.g. https://github.com/blakeembrey/jaywalk/blob/master/src/types/index.ts or https://github.com/blakeembrey/node-scrappy/blob/master/src/index.ts).

@unional
Copy link
Contributor Author

unional commented Jul 8, 2016

I had assumed this was fixed, but I guess I reviewed it incorrectly

I looked it up and it fixed the export default and augmentation in ADDING interface: #8840
image
image

But this is about augmenting existing interface (i.e. merging).

@unional
Copy link
Contributor Author

unional commented Jul 8, 2016

@sandersn
Copy link
Member

sandersn commented Jul 8, 2016

It's a technical limitation. I don't know yet if it's by design (and if so, if there's an intended workaround) -- I'm waiting on @RyanCavanaugh to get back from vacation since he knows much more than I do about typings.

@sandersn
Copy link
Member

Unfortunately, after some more discussion with the team, we decided that we can't support this pattern for now. We might revisit this decision in the future, but current DefinitelyTyped code has worked around this limitation so far, which may indicate that it's (1) easy to work around or (2) not that common.

@sandersn sandersn added the Design Limitation Constraints of the existing architecture prevent this from being fixed label Jul 27, 2016
@blakeembrey
Copy link
Contributor

@sandersn Unless I've misunderstood something, isn't this extremely common? Basically any node module that's re-exporting from multiple files follows this pattern (which is most node modules, you can regularly find anything bigger than a single function util exporting multiple files in index.js). I believe even TypeScript would be authored in this format if it were originally written as ES6 modules.

DefinitelyTyped "works around it" because it mutates the global scope. The only other possible work-around in ordinary code would be to augment the original source file. But that means any file in a module is now part of the public API, which is frowned upon doing and quite fragile.

@sandersn
Copy link
Member

@RyanCavanaugh Thoughts? You've worked with DefinitelyTyped a lot more than I have.

@unional
Copy link
Contributor Author

unional commented Aug 11, 2016

@RyanCavanaugh what's your thought?

I have a private library written in TypeScript that hit this limitation.
And example is to imagine redux is written in TypeScript and redux-thunk and redux-promise needs to augment the Dispatch method.

This bug prevents that from happening.

My workaround is put all source code into index.ts in the "redux" package. Which definitely won't scale as the package evolves.

IMO this bug needs to be re-opened.

@unional
Copy link
Contributor Author

unional commented Sep 26, 2016

Pinging @RyanCavanaugh

@unional
Copy link
Contributor Author

unional commented Nov 9, 2016

Any update on this? We should reconsider this, not just for typings, for package written in TypeScript in general.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Design Limitation Constraints of the existing architecture prevent this from being fixed
Projects
None yet
Development

No branches or pull requests

5 participants