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

export [default] class A {} generates invalid ES6 module code #5790

Closed
Victorystick opened this issue Nov 25, 2015 · 13 comments
Closed

export [default] class A {} generates invalid ES6 module code #5790

Victorystick opened this issue Nov 25, 2015 · 13 comments

Comments

@Victorystick
Copy link

Running TypeScript version 1.7.4 (the Playground version) with the following options

ts.transpileModule(..., {
  compilerOptions: {
    target: ts.ScriptTarget.ES5,
    module: ts.ModuleKind.ES6
  }
}).outputText

yields incorrect outputs in these two situations:

export class A {}
export default class A {}

// results in these snippets, respectively

var A = (function () {
    function A() {
    }
    return A;
})();
A = A;
var A = (function () {
    function A() {
    }
    return A;
})();
exports.default = A;

when I'd expect

export var A = (function () {
    function A() {
    }
    return A;
})();
export default (function () {
    function A() {
    }
    return A;
})();

Oddly enough, this seems to work as expected:

class A {}
export { A };
export default A;

// generates

var A = (function () {
    function A() {
    }
    return A;
})();
export { A };
export default A;

(See rollup/rollup-plugin-typescript#9 for the original discussion.)

@Victorystick
Copy link
Author

This may be related to #5594, and possibly (at least partially) fixed by #5648 (solves the opposite case; ES6 code with pre-ES modules).

@vladima
Copy link
Contributor

vladima commented Nov 25, 2015

note that original example is not legal ES6 code: export default class A {}introduces export named default as well as local name A which will collide with class A declared above. Your workaround is actually the right way to go, alternatively you can use:

export default class A {}
export {A}

or

export class A {}
export default A

@vladima vladima closed this as completed Nov 25, 2015
@Victorystick
Copy link
Author

I wasn't being clear enough. What I meant is that

export default class A {}

transpiles to

var A = (function () {
    function A() {
    }
    return A;
})();
exports.default = A;

although the target module syntax is supposed to be ES6, where you'd expect something like

var A = (function () {
    function A() {
    }
    return A;
})();
export default A;

to be generated. TypeScript is mixing CommonJS with ES2015 although the latter was requested.

@vladima
Copy link
Contributor

vladima commented Nov 25, 2015

This scenario is not supported, attempt to compile this code from the command line yields

error TS1204: Cannot compile modules into 'es2015' when targeting 'ES5' or lower.

transpile* currently reports only syntax errors (assuming that other errors are semantic and thus cannot be 100% trusted) however I think we should report errors that are produced during compiler option validation - #5798 tracks this.

@vladima
Copy link
Contributor

vladima commented Nov 26, 2015

update: we correctly fill diagnostics property on the output result if reportDiagnostics on input transpileOptions is set to true. This code

var ts = require("./built/local/typescript");
var text = "export default class A {}";
var r = ts.transpileModule(text, {
  reportDiagnostics: true,
  compilerOptions: {
    target: ts.ScriptTarget.ES5,
    module: ts.ModuleKind.ES6
  }
});
console.log(r)

yields

{ outputText: 'var A = (function () {\n    function A() {\n    }\n    return A;\n})();\nexports.default = A;\n',
  diagnostics: 
   [ { file: undefined,
       start: undefined,
       length: undefined,
       messageText: 'Cannot compile modules into \'es2015\' when targeting \'ES5\' or lower.',
       category: 1,
       code: 1204 } ],
  sourceMapText: undefined }

@weswigham
Copy link
Member

Though if enough people see a need for it, we could conceivably support the configuration - it's just not something our emitter currently expects to need to do.

@Victorystick
Copy link
Author

@vladima Thanks for reportDiagnostics; I was missing that.

@weswigham Is there any reason not to allow ES2015 modules with an ES5 or ES3 compile target? Practically all other configurations seem to be supported. Such a configuration should allow Rollup to bundle TypeScript apps, without having to rely on a separate pass just to transpile the generated ES2015 code to ES5.

I've modified emitter.ts to emit ES2015 export code for classes. While I'm not very familiar with TypeScript's test suite, I could try to add some tests for it.

@pkozlowski-opensource
Copy link

Though if enough people see a need for it, we could conceivably support the configuration - it's just not something our emitter currently expects to need to do.

@weswigham IMO there are 2 options:

  • ES6 modules are supported for ES5 target and we can use external packagers / tree-shakers
  • TypeScript provides build-in packager with tree-shaking capabilities.

I don't know what is easier from the technical point of view nor what is TS longer-term strategy / vision for packaging / bundling / tree-shaking but IMO splitting those efforts and let external bundlers do the job might be a way to go.

@kitsonk
Copy link
Contributor

kitsonk commented Nov 27, 2015

  • TypeScript provides build-in packager with tree-shaking capabilities.

Outside of the potential of bundling, as far as the tree-shaking, that is specifically against the design goals of TypeScript and been discussed before and likely won't ever arrive.

  • ES6 modules are supported for ES5 target and we can use external packagers / tree-shakers

If your external packager / tree-shaker only supported ES6 modules, I would get a different one. I can see the value in outputting ES5 code in an ES6 module format, but I can't think of a single mainstream module loader that only supports ES6 modules, because there is not agreement on the ES6 module loading semantics. So I see it as a nice to have at this point versus a requirement.

Why do you feel that ES6 modules are a barrier to using an external packager / tree-shaker?

@pkozlowski-opensource
Copy link

If your external packager / tree-shaker only supported ES6 modules, I would get a different one.

@kitsonk we are talking abut https://github.com/rollup/rollup here. Are you aware of any other packager that does (or at least tries) real tree-shaking?

@kitsonk
Copy link
Contributor

kitsonk commented Nov 27, 2015

Sorry, I am clearly out of touch with the internet for 6-7 weeks since rollup came into existence. It seems the wheel of re-invention comes around again. I think the assertion though that tree shaking/dead code removal is not unique to rollup. Instead of improving WebPack or Browserify, we go off and invent something new. 😢 (Also with dead code removal/tree shaking coming in WebPack2) Also, its assertion that this can only be done under ES6 module format I find confusing, but oh well... In addition, since MIDs, dependency resolution, etc. are still up in the air as far as ES6, I think it is very risky to adopt ES6 modules as the final state for individual projects at the moment, but that is my opinion.

I do agree with the import syntax of ES6, it does make it a bit easier to identify sub-module unused code.

As far as other tooling that does tree shaking/dead code removal, irrespective of the module format:

Admittedly, you need a level of other tooling to create the layers/bundles for your application. Clearly tooling in JavaScript these days is a bit of a challenge.

@pkozlowski-opensource
Copy link

@kitsonk I'm too familiar with the lamentable state of ES6 loaders / build tools. But let's not side-track here - the issue here is very specific.

I do agree with the import syntax of ES6, it does make it a bit easier to identify sub-module unused code.

Precisely. And afaik WebPack2 will also work off the ES6 modules.

So the practical question is: how do we feed ES6-modules code into those tools. Of course I could use:
TS -> Rollup (or whatever) -> Babel (or anything that can do ES6 to ES5) but it makes the whole setup even more complex.

Regarding other mentioned tools:

  • I don't think that UglisfyJS aims at the same level of tree-shaking as rollup
  • from my conversations with people from Google about closure compiler it doesn't seem like it is easy / usable in practice right now

@kitsonk
Copy link
Contributor

kitsonk commented Nov 27, 2015

So the practical question is: how do we feed ES6-modules code into those tools. Of course I could use:
TS -> Rollup (or whatever) -> Babel (or anything that can do ES6 to ES5) but it makes the whole setup even more complex.

Yes, I was getting side tracked. My personal opinion is that there is no harm in emitting ES5 code in ES6 module format (though my opinion is far from official).

Personal experience is that UglifyJS does static analysis and does a decent job of removing unreachable code. It isn't specifically aware of the "easy" pruning that could be done with ES6 module important syntax and I will admit I haven't tried it. Most of my experience is with AMD.

To get the right level of tree shaking you are looking for from Closure isn't available without advanced optimisations, which yes, are usually rather impractical for most people with a lot of hand tuning or writing your code in very specific ways up front.

Thinking out loud though, someone creating a tree-shaking/bundling tool on top of TypeScript compiler services isn't the worst thing in the world. I think the compiler services would be able to surface up everything to make pruning decisions. Clearly it is something the TypeScript team don't want to tackle (and I don't blame them) but there is clearly a need in the community and the raw tools appear to be there. I would suspect that it would be far more effective than rollup. thinking

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants