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

Implicit module import/export elision #2812

Closed
mhegazy opened this Issue Apr 17, 2015 · 16 comments

Comments

Projects
None yet
5 participants
@mhegazy
Contributor

mhegazy commented Apr 17, 2015

Current behavior

The compiler today elides imports and exports that are types or not referenced in a value position in the body of the importing module. See spec sections section 11.2.5 and section 11.2.6 for more details.

This has allowed for using the same syntax to import types and/or namespaces the same way values are imported. This has been convenient but has caused some problems:

  1. It has been consistently a source of confusion for TypeScript users (see issues: #2132, #2038, and #596).
  2. It impedes single-file-transpilation (see #2499) as there is no way to know if a re-export (e.g. t in export { t } from "mod") should be elided or not without looking at the whole program

Proposed change

1. Do not elide module imports even if they are only used in type position.

Even if the import is not used, keep the module dependency (i.e. the require() call) to ensure any side effects are maintained.
e.g.:

import { InterfaceFoo } from "foo";
import { ClassBar } from "bar";

var x: InterfaceFoo = new ClassBar();

emits in ES6:

import {} from "foo";
import { ClassBar } from "bar";
var x = new ClassBar();

and ES5/commonjs:

require("foo"); // import but do not capture
var bar_1 = require("bar");
var x = new bar_1.ClassBar();
2. Exports with no value side are always elided

This is the existing behavior, an export to an interface or a non-instantiated module will be elided.

e.g.:

interface I {

}

export { I }; // Elided along with the interface declaration
3. A type modifier is required for type-only imports and exports

The type modifier will cause the import to alias the type side of the imported entity, and would indicate the intent for this import statement to be always elided.

import type { IFoo } from "mod1"; // import to "mod1" will be elided

export type { IBar } from "mod2"; // similarly, export statement will be elided

Optionally type can be applied to specific named bindings in export and import declarations, which will result in eliding the import if all its bindings are types.

import {type IFoo, type IBar} from "mod"; // import will be elided

Errors:

  • It is an error to use type on a value-only import/export (e.g. var declaration).
  • It is an error to import or re-export a type only name without a type modifier
import { Interface, Class, Enum } from "foo"; /// Error: import `Interface` does not have a value side 
                                              ///        and should be marked as type.

export { type } from "bar";                   /// Error: export `type` does not have a value side 
                                              ///        and should be marked as type.
  • Similarly default exports must have a value side:
interface IFoo {
}
export default IFoo; /// Error: export default target must have a value.

Implications to import = / export = syntax

The proposal only affects the new ES6-style import/export syntax. Existing elision rules for import id = require("module") and export = id are not changed.

@poelstra

This comment has been minimized.

Show comment
Hide comment
@poelstra

poelstra Apr 21, 2015

Similarly default exports must have a value side:

A few days ago, I created a file like:

/// Thenable.ts
interface Thenable<T> {
  // then() method definition
}
export default Thenable;

Then in another:

/// foo.ts
import Thenable from "./Thenable";
var p: Thenable = ...;

I wouldn't be against your suggestion to change foo.ts to: import type Thenable from "./Thenable";, but I'm not sure why you want to prevent the type-only default export completely?

BTW, isn't requireing the "type" keyword for importing types also already a breaking change, given that people seem to be using 1.5.0-alpha a lot already?

poelstra commented Apr 21, 2015

Similarly default exports must have a value side:

A few days ago, I created a file like:

/// Thenable.ts
interface Thenable<T> {
  // then() method definition
}
export default Thenable;

Then in another:

/// foo.ts
import Thenable from "./Thenable";
var p: Thenable = ...;

I wouldn't be against your suggestion to change foo.ts to: import type Thenable from "./Thenable";, but I'm not sure why you want to prevent the type-only default export completely?

BTW, isn't requireing the "type" keyword for importing types also already a breaking change, given that people seem to be using 1.5.0-alpha a lot already?

@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Apr 21, 2015

Contributor

yes that would be a breaking change. marking it.

Contributor

mhegazy commented Apr 21, 2015

yes that would be a breaking change. marking it.

@jbondc

This comment has been minimized.

Show comment
Hide comment
@jbondc

jbondc Apr 22, 2015

Contributor

Agree about export default not having a value, instead you can write:

export interface Thenable<T> {
  // then() method definition
}
export type someString = string;

Symmetry on import:

// import interface {interfaceDeclarationsList}
import interface {Thenable} from "./Thenable";
// import type {typeAliasList}
import type {someString} from "./Thenable";

IMHO, both 'import interface' and 'import type' make it more intuitive.

Contributor

jbondc commented Apr 22, 2015

Agree about export default not having a value, instead you can write:

export interface Thenable<T> {
  // then() method definition
}
export type someString = string;

Symmetry on import:

// import interface {interfaceDeclarationsList}
import interface {Thenable} from "./Thenable";
// import type {typeAliasList}
import type {someString} from "./Thenable";

IMHO, both 'import interface' and 'import type' make it more intuitive.

@poelstra

This comment has been minimized.

Show comment
Hide comment
@poelstra

poelstra Apr 22, 2015

@jon Yeah, I agree with import type (don't know why you'd need import interface though?), but I'm wondering what the reason is for explicitly disallowing it for the default export.

Given that the ES6 committee has really tried hard to make default imports/exports easy to use, I expect them to be used a lot.

So, imports will often look like:

import Foo from "./Foo";
import Bar from "./Bar";
import type Baz from "./Baz";
// etc.

To me, that looks cleaner than:

import Foo from "./Foo";
import Bar from "./Bar";
import type { Baz } from "./baz";

Syntax for the default export could e.g. look like:

interface Baz { ... }
export default type Baz;

@mhegazy I'm probably missing something very obvious here, can you comment on why you specifically want to prohibit default type exports?

Would the following still be allowed?

interface Baz { ... }
export { type Baz as default };

Or even

import { type default as Baz } from "./Baz";

poelstra commented Apr 22, 2015

@jon Yeah, I agree with import type (don't know why you'd need import interface though?), but I'm wondering what the reason is for explicitly disallowing it for the default export.

Given that the ES6 committee has really tried hard to make default imports/exports easy to use, I expect them to be used a lot.

So, imports will often look like:

import Foo from "./Foo";
import Bar from "./Bar";
import type Baz from "./Baz";
// etc.

To me, that looks cleaner than:

import Foo from "./Foo";
import Bar from "./Bar";
import type { Baz } from "./baz";

Syntax for the default export could e.g. look like:

interface Baz { ... }
export default type Baz;

@mhegazy I'm probably missing something very obvious here, can you comment on why you specifically want to prohibit default type exports?

Would the following still be allowed?

interface Baz { ... }
export { type Baz as default };

Or even

import { type default as Baz } from "./Baz";
@jbondc

This comment has been minimized.

Show comment
Hide comment
@jbondc

jbondc Apr 22, 2015

Contributor

For me it's more about not having too much syntax for importing.

This seems enough:

import type { Baz, Foo }; // (a)

Are these really needed? Adds visibility clutter if people use in different ways:

import { type Baz, type Foo }; // (b)
import type Baz; // (c)

For the 'import interface' that's just a personal preference for symmetry:
http://en.wikipedia.org/wiki/Symmetry

Contributor

jbondc commented Apr 22, 2015

For me it's more about not having too much syntax for importing.

This seems enough:

import type { Baz, Foo }; // (a)

Are these really needed? Adds visibility clutter if people use in different ways:

import { type Baz, type Foo }; // (b)
import type Baz; // (c)

For the 'import interface' that's just a personal preference for symmetry:
http://en.wikipedia.org/wiki/Symmetry

@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Apr 22, 2015

Contributor

As @jbondc explained; this was the intention; trying to limit the additional syntax to minimum, in theory we can make default take a type modifier as well, but then that adds another entry to the matrix.

Contributor

mhegazy commented Apr 22, 2015

As @jbondc explained; this was the intention; trying to limit the additional syntax to minimum, in theory we can make default take a type modifier as well, but then that adds another entry to the matrix.

@poelstra

This comment has been minimized.

Show comment
Hide comment
@poelstra

poelstra Apr 22, 2015

@mhegazy Feared as much, which is why I asked whether the 'alternative' default syntax would be allowed. Thanks for the confirmation that it's indeed 'just' the additional syntax.

Given ES6's 'push' for the default syntax, as a user, I think it would be worth the trouble to add the 'entry to the matrix', but then again, I don't feel the pain of adding/maintaining it in the compiler ;)

@jbondc As @mhegazy stated in his proposal, the import { type Baz, type Foo } case is useful for when you want to import e.g. both classes and interfaces in one line.
Note that I agree with you on the symmetry, it's just that I wanted to address that many people are probably going to use the default syntax for a lot of libraries, so there will be a lot of imports without the curly-braces. And because import { Baz } ... means something different from import Baz ..., it's not up to the 'user' of the library to decide to e.g. consistently use just the curly-braces.

poelstra commented Apr 22, 2015

@mhegazy Feared as much, which is why I asked whether the 'alternative' default syntax would be allowed. Thanks for the confirmation that it's indeed 'just' the additional syntax.

Given ES6's 'push' for the default syntax, as a user, I think it would be worth the trouble to add the 'entry to the matrix', but then again, I don't feel the pain of adding/maintaining it in the compiler ;)

@jbondc As @mhegazy stated in his proposal, the import { type Baz, type Foo } case is useful for when you want to import e.g. both classes and interfaces in one line.
Note that I agree with you on the symmetry, it's just that I wanted to address that many people are probably going to use the default syntax for a lot of libraries, so there will be a lot of imports without the curly-braces. And because import { Baz } ... means something different from import Baz ..., it's not up to the 'user' of the library to decide to e.g. consistently use just the curly-braces.

@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Apr 22, 2015

Contributor

Given ES6's 'push' for the default syntax, as a user, I think it would be worth the trouble to add the 'entry to the matrix', but then again, I don't feel the pain of adding/maintaining it in the compiler ;)

my argument is if you have a value, then there is no issue, which is the common case (i.e. class, function, var, module). it only breaks down if you only have an interface or a type alias, which really does not exist in the .js anyways.. so for these you can do the slightly longer version of export type { i as default }

Contributor

mhegazy commented Apr 22, 2015

Given ES6's 'push' for the default syntax, as a user, I think it would be worth the trouble to add the 'entry to the matrix', but then again, I don't feel the pain of adding/maintaining it in the compiler ;)

my argument is if you have a value, then there is no issue, which is the common case (i.e. class, function, var, module). it only breaks down if you only have an interface or a type alias, which really does not exist in the .js anyways.. so for these you can do the slightly longer version of export type { i as default }

@RyanCavanaugh

This comment has been minimized.

Show comment
Hide comment
@RyanCavanaugh

RyanCavanaugh Jun 8, 2015

Member

Summary: It will be an error to re-export a type-only identifier from an import in ES6 in --isolatedModules mode.

Member

RyanCavanaugh commented Jun 8, 2015

Summary: It will be an error to re-export a type-only identifier from an import in ES6 in --isolatedModules mode.

@poelstra

This comment has been minimized.

Show comment
Hide comment
@poelstra

poelstra Jun 9, 2015

@RyanCavanaugh Do you mean that the outcome of other discussions have basically obsoleted/replaced all of the OP's proposal, and this is the only part that remains?

We'd like to know, because we would like to start using ES6-style imports, but are holding off until this settles.

poelstra commented Jun 9, 2015

@RyanCavanaugh Do you mean that the outcome of other discussions have basically obsoleted/replaced all of the OP's proposal, and this is the only part that remains?

We'd like to know, because we would like to start using ES6-style imports, but are holding off until this settles.

@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Jun 9, 2015

Contributor

Do you mean that the outcome of other discussions have basically obsoleted/replaced all of the OP's proposal, and this is the only part that remains?

yes. this is correct, there are no intended changes/additions to the module syntax. the reaming work from this suggestion is to flag an error when using --target ES6, --isolatedModules, and exporting a non-locally defined type. if you are not using any of these, there should not be any impact on your code.

Contributor

mhegazy commented Jun 9, 2015

Do you mean that the outcome of other discussions have basically obsoleted/replaced all of the OP's proposal, and this is the only part that remains?

yes. this is correct, there are no intended changes/additions to the module syntax. the reaming work from this suggestion is to flag an error when using --target ES6, --isolatedModules, and exporting a non-locally defined type. if you are not using any of these, there should not be any impact on your code.

@poelstra

This comment has been minimized.

Show comment
Hide comment
@poelstra

poelstra Jun 10, 2015

Nice, thanks!

poelstra commented Jun 10, 2015

Nice, thanks!

@RyanCavanaugh

This comment has been minimized.

Show comment
Hide comment
@RyanCavanaugh

RyanCavanaugh Aug 5, 2015

Member

Sounds like we're not doing any other work on this issue. Is that correcy @mhegazy ?

Member

RyanCavanaugh commented Aug 5, 2015

Sounds like we're not doing any other work on this issue. Is that correcy @mhegazy ?

@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Aug 5, 2015

Contributor

yup. this was declined last time we talked about it.

Contributor

mhegazy commented Aug 5, 2015

yup. this was declined last time we talked about it.

@mhegazy mhegazy closed this Aug 5, 2015

@mhegazy mhegazy added Declined and removed In Discussion labels Aug 5, 2015

@mhegazy

This comment has been minimized.

Show comment
Hide comment
@mhegazy

mhegazy Aug 5, 2015

Contributor

The rational is that the import syntax is already complex enough, and no need to add more complexity.

Contributor

mhegazy commented Aug 5, 2015

The rational is that the import syntax is already complex enough, and no need to add more complexity.

@philkunz

This comment has been minimized.

Show comment
Hide comment
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.