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

Implicit module import/export elision #2812

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

Implicit module import/export elision #2812

mhegazy opened this issue Apr 17, 2015 · 16 comments
Labels
Breaking Change Would introduce errors in existing code Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript

Comments

@mhegazy
Copy link
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: Imports left out of module dependencies when not directly assigned or assigned to. #2132, define parameter not generated for a referenced AND used external module class. #2038, and force import #596).
  2. It impedes single-file-transpilation (see Support single-module transpilation mode #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.

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Apr 17, 2015
@poelstra
Copy link

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
Copy link
Contributor Author

mhegazy commented Apr 21, 2015

yes that would be a breaking change. marking it.

@mhegazy mhegazy added the Breaking Change Would introduce errors in existing code label Apr 21, 2015
@jbondc
Copy link
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
Copy link

@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
Copy link
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
Copy link
Contributor Author

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
Copy link

@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
Copy link
Contributor Author

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
Copy link
Member

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

@poelstra
Copy link

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
Copy link
Contributor Author

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
Copy link

Nice, thanks!

@RyanCavanaugh
Copy link
Member

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

@mhegazy
Copy link
Contributor Author

mhegazy commented Aug 5, 2015

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

@mhegazy mhegazy closed this as completed Aug 5, 2015
@mhegazy mhegazy added Declined The issue was declined as something which matches the TypeScript vision and removed In Discussion Not yet reached consensus labels Aug 5, 2015
@mhegazy
Copy link
Contributor Author

mhegazy commented Aug 5, 2015

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

@philkunz
Copy link

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 Declined The issue was declined as something which matches the TypeScript vision Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

5 participants