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

Reopening external modules broken in TS 1.5-alpha #2784

Closed
basarat opened this issue Apr 15, 2015 · 11 comments
Closed

Reopening external modules broken in TS 1.5-alpha #2784

basarat opened this issue Apr 15, 2015 · 11 comments
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead

Comments

@basarat
Copy link
Contributor

basarat commented Apr 15, 2015

Consider a.d.ts (which compiles fine in isolation) without any error:

declare module "foo" {
    interface Foo {
        a: number;
    }
    var _:Foo;
    export = _;
}

And b.d.ts that plans to add stuff to module foo's Foo interface:

/// <reference path="./a"/>

declare module "foo" {
    interface Foo {
        b: number;
    }
}

With this we have two errors:

  • a.d.ts gets an error on export = "An export assignment cannot be used in a module with other exported elements.
  • b.d.ts doesn't actually manage to add to the interface Foo as demonstrated by a test file test.ts:
/// <reference path="./a"/>
/// <reference path="./b"/>

import foo = require("foo");
foo.b = 123; // ERROR: Property b to not exist on type Foo

Discovered in DefinitelyTyped : DefinitelyTyped/DefinitelyTyped#4101 /cc @vvakame

basarat added a commit to DefinitelyTyped/DefinitelyTyped that referenced this issue Apr 16, 2015
@danquirk danquirk added the Bug A bug in TypeScript label Apr 16, 2015
@ahejlsberg
Copy link
Member

Don't think this is a bug. It isn't possible to have other exports in a module that uses export = so the two declarations of Foo are considered separate local declarations that don't merge. It is possible that earlier versions of the compiler permitted this (we tightened things up in this area as part of the ES6 module work), but it was never intended behavior.

I don't see the error "An export assignment cannot be used in a module with other exported elements" and I wouldn't expect to as there are no other exported elements.

I do see the error "Property 'b' does not exist on type 'typeof "foo"'" and I think that is correct.

basarat added a commit to TypeStrong/atom-typescript-examples that referenced this issue Apr 16, 2015
@basarat
Copy link
Contributor Author

basarat commented Apr 16, 2015

It is possible that earlier versions of the compiler permitted this (we tightened things up in this area as part of the ES6 module work), but it was never intended behavior.

Cool. I can verify that it did work.

I don't see the error "An export assignment cannot be used in a module with other exported elements"

It might just be the language service :

image

I've uploaded the sample for testing : https://github.com/TypeStrong/atom-typescript-examples/tree/master/errorcases/reopenExternalModules


This brings us back to the question: If I write an npm module and create a definition:

declare module "foo" {
    interface Foo {
        a: number;
    }
    var _:Foo;
    export = _;
}

How can someone create another NPM module and provide enhancements for my Foo. Perhaps the answer is to use non instantiated modules in my definition so that they can be safely reopened and extended. Correct?

@ahejlsberg
Copy link
Member

I'm not entirely clear on what you're trying to model here, but certainly one way you could do it is to declare the Foo interface in the global scope (or in an uninstantiated internal module in the global scope). For example a.d.ts:

declare module FooTypes {
    export interface Foo {
        a: number;
    }
}

declare module "foo" {
    var _: FooTypes.Foo;
    export = _;
}

And b.d.ts:

/// <reference path="./a"/>

declare module FooTypes {
    export interface Foo {
        b: number;
    }
}

Now, with my ES6 hat on, the above isn't a pattern I would recommend. In ES6, modules are non-extensible objects and you can't just jam properties onto them. Instead you declare a new module and use export ... from "foo" to re-export some or all of the existing module's members.

@mhegazy mhegazy added By Design Deprecated - use "Working as Intended" or "Design Limitation" instead and removed Bug A bug in TypeScript labels Apr 16, 2015
@yjo
Copy link

yjo commented Aug 5, 2015

I'm seeing this same issue (in tsc 1.5.3) and I'd be very interested to hear of any workarounds there might be. I'm using a DefinitelyTyped declaration that has been written similarly to @basarat's example:

declare module "foo"
{
    module foo {
        interface Foo {
            bar: Bar;
        }
        interface Bar {
            a: number;
        }
    }
    var foo: foo.Foo;
    export = foo;
}

The problem is that one of the TSD module interfaces (Bar) is missing a member that I need to use. Of course the medium-long term solution would be to create a PR for DefinitelyTyped that addresses the issue and reference it once it gets merged, but I'm hoping there's a solution that lets me re-open the interface to add what I need in a separate file.

In my case the error TS2309: An export assignment cannot be used in a module with other exported elements is flagged at the export = foo line of the TSD definition, and not within my extension .d.ts file, which looks like:

declare module "foo" {
    interface Bar{
        b: number;
    }
}

Is there another way to add to this interface without modifying the main TSD definition file?

@mhegazy
Copy link
Contributor

mhegazy commented Aug 5, 2015

Is there another way to add to this interface without modifying the main TSD definition file?

I do not see other way around this.

@basarat
Copy link
Contributor Author

basarat commented Aug 6, 2015

I do not see other way around this.

Same here. We went with wrapping those interfaces in a non instantiated module (i.e. modifying the original definitions)

@yjo
Copy link

yjo commented Aug 6, 2015

@mhegazy @basarat having types that are closed 'by design' doesn't feel very TypeScripty to me... It's hard enough to persuade a team of TS's merits without having to sprinkle around the odd <any> because the library types are incomplete .

@danquirk
Copy link
Member

danquirk commented Aug 6, 2015

Can you not just edit your local copy of the .d.ts until the PR for the fix is taken?

@yjo
Copy link

yjo commented Aug 6, 2015

@danquirk it would be possible, but on my team we try to avoid checking in library code, and it would be pretty icky to check in a fork of a huge definitions file just to add a single line.

@Haemoglobin
Copy link

I need to do this in order to use react-css-modules (https://github.com/gajus/react-css-modules), as it dynamically adds a property called styleName to React's HTMLAttributes definition.

React definitions should not be extended to include the styleName attribute, as its the separate react-css-modules library that extends this. So it should be possible to extend interfaces in typescript modules (it's not just a matter of waiting for a PR to the definitions to be merged).

I'm actually not sure how I use the react-css-modules library with typescript without modifying the react definitions? Do you have any tips @ahejlsberg ?

Thanks!

@DanielRosenwasser
Copy link
Member

I believe we are modeling this scenario with #6213.

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead
Projects
None yet
Development

No branches or pull requests

7 participants