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

[Spec] Duplicate ambient external module name #52

Closed
mhegazy opened this issue Jul 17, 2014 · 8 comments
Closed

[Spec] Duplicate ambient external module name #52

mhegazy opened this issue Jul 17, 2014 · 8 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Spec Issues related to the TypeScript language specification

Comments

@mhegazy
Copy link
Contributor

mhegazy commented Jul 17, 2014

declare module "fs" {
    var x: string;
}
declare module "fs" {
    var y: number;
}

Expected:
!!! Duplicate identifier ''fs''.
!!! Ambient external module declaration cannot be reopened.
Actual: no error

@mhegazy mhegazy added this to the TypeScript 1.1 milestone Jul 17, 2014
@ahejlsberg
Copy link
Member

Changing to spec issue. The new compiler supports merging of all module declarations, including ambient external module declarations. I see no reason why we should only allow merging of internal modules and specifically disallow merging of ambient external modules. In particular, declaration files sometimes extend existing internal modules and may want to do exactly that for ambient external modules.

@ahejlsberg ahejlsberg changed the title [Errors] Duplicate ambient external module name [Spec] Duplicate ambient external module name Jul 17, 2014
@ahejlsberg ahejlsberg reopened this Jul 17, 2014
@sophiajt
Copy link
Contributor

From design meeting: agreed this should merge just like internal modules.

@sophiajt sophiajt reopened this Jul 18, 2014
@sophiajt
Copy link
Contributor

Leaving open so we can make the text change to the spec

@iislucas
Copy link

When I run this code in typescript I get

ldixon/foo.ts(4,16): error TS2000: Duplicate identifier '"fs"'. Additional locations:
    ldixon/foo.ts(1,16)
ldixon/foo.ts(4,16): error TS2191: Ambient external module declaration cannot be reopened.

Sounds like the correct behaviour is no errors. Would be great to have an npm publish of a version with this fixed :)

@basarat
Copy link
Contributor

basarat commented Jul 23, 2014

agreed this should merge just like internal modules.

DT actually needs this. Thanks guys (/cc @Bartvds @vvakame will possibly make non-instantiated modules less necessary)

@spion
Copy link

spion commented Dec 1, 2015

It seems like this doesn't work if the module is exported by assignment?

declare module "m" {
  function inner(s: string);
  module inner { export interface Test {t: string;} }
  export = inner //error here
}

declare module "m" {
  // even though we're only extending already existing submodule
  module inner { export interface Other { t: number; } }
}

@RyanCavanaugh
Copy link
Member

@spion The name inner is not "shared" between declarations here -- when there's an export =, all other declarations inside that module declaration are considered to not be visible at all

@spion
Copy link

spion commented Dec 1, 2015

@RyanCavanaugh I see. The original problem is express.d.ts

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/express/express.d.ts

Here, express exposes a namespace Express to let us extend Request. Unfortunately those extensions cannot contain types imported from ambient external modules, which means they're of limited use. However, since express-the-module is also a function, the only way to declare the module is to use declaration merging then write export = module at the end, so we can't use declaration merging for ambient external modules either.

I'll open a separate issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue Spec Issues related to the TypeScript language specification
Projects
None yet
Development

No branches or pull requests

7 participants