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

Compiler should error when merging with a default exported declaration #3095

Closed
DanielRosenwasser opened this issue May 9, 2015 · 8 comments
Assignees
Labels
Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@DanielRosenwasser
Copy link
Member

Currently if you have the following file m1.ts

export default function Decl() {
    return 0;
}

export interface Decl {
    p1: number;
    p2: number;
}

export namespace Decl {
    export var x = 10;
    export var y = 20;

    interface I {
    }
}

trying to import m1.ts with a default binding will not give you all meanings of the exported entity:

import Entity from "m1"

Entity();

var x: Entity; // error
var y: Entity.I; // error

Entity.x; // error
Entity.y; // error

Discussed this with @JsonFreeman and @vladima, will try to elaborate more, but this issue should serve as a discussion point for suggestions, since there are certain interesting cases like non-exported uninstantiated namespaces.

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Discussion Issues which may not have code impact labels May 9, 2015
@ahejlsberg
Copy link
Member

I think the real error here is that there are multiple declarations for the same name with different modifiers, i.e. the first declaration is export default and the other two are just export. We should disallow that, just as we disallow a set of declarations where some are export and others aren't.

That of course raises the question of whether to allow export default with declarations other than class and function. I don't see a good reason to. I think we're better off just recommending you use this pattern:

function Foo() {
   // ...
}
namespace Foo {
   // ...
}
interface Foo {
   // ...
}
export default Foo;

@DanielRosenwasser
Copy link
Member Author

@JsonFreeman and I had the idea of actually making

export default function Foo() {
    // ...
}
namespace Foo {
    // ...
}
interface Foo {
    // ...
}

equivalent to

function Foo() {
    // ...
}
namespace Foo {
    // ...
}
interface Foo {
    // ...
}
export default Foo;

Though I haven't had the chance to give it much thought.

Overall I'm not completely against erroring since it affords us future flexibility.

@JsonFreeman
Copy link
Contributor

Well my idea was to say that all exports of the local symbol Foo would be visible from the outside as both Foo and default. So like this:

export default function Foo() { // visible outside as either default or Foo
    // ...
}
export interface Foo { // visible outside as either default or Foo
    // ...
}
namespace Foo { // not visible outside
    // ...
}

However, my idea won't work because we do not emit anything for the interface declaration, even though it would have the runtime effect of exporting the function Foo as Foo.

@ahejlsberg I like the principle of your idea, but it is actually not the same as what we do for the 'export' modifier. For example we allow you to do this:

function Foo() { }
export interface Foo { }

but not this:

function Foo() { }
export module Foo {
   export var x;
}

The rule would be that all declarations of the same name that occupy overlapping declaration spaces need to have the same modifiers.

Although actually now that I think about it, we could indeed follow this same pattern for default. If we do not allow a default and a non-default to have overlapping declaration spaces, then it will work to just export two different symbols for them. Because in this case, you can never use the default export in a position that would have access to meanings from a non-default export, and vice versa.

@JsonFreeman
Copy link
Contributor

So in summary @DanielRosenwasser's initial example would be an error.

@DanielRosenwasser DanielRosenwasser changed the title 'export default' declarations do not merge correctly Compiler should error when merging with a default exported declaration May 11, 2015
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 1.5.2 milestone May 16, 2015
@DanielRosenwasser DanielRosenwasser self-assigned this May 16, 2015
@mhegazy mhegazy modified the milestones: TypeScript 1.6, TypeScript 1.5.2 May 19, 2015
@DanielRosenwasser DanielRosenwasser added Breaking Change Would introduce errors in existing code and removed Discussion Issues which may not have code impact labels Jul 22, 2015
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Jul 27, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Jul 27, 2015

@DanielRosenwasser can you add a blurb for this in breaking changes section.

@DanielRosenwasser
Copy link
Member Author

@DanielRosenwasser
Copy link
Member Author

@ahejlsberg I think the spec might need to reflect this.

@mhegazy
Copy link
Contributor

mhegazy commented Oct 6, 2015

@DanielRosenwasser please create a new spec issue for this.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Breaking Change Would introduce errors in existing code Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

4 participants