Support modules when targeting ES6 and an ES6 ModuleKind #4811
Conversation
| @@ -76,10 +76,11 @@ namespace ts { | |||
| "amd": ModuleKind.AMD, | |||
| "system": ModuleKind.System, | |||
| "umd": ModuleKind.UMD, | |||
| "es6": ModuleKind.ES6, | |||
There was a problem hiding this comment.
you do not need that. just leave target as es6 and allow for additional --module flag. today we make it an error to use --target es6 with --module; relaxing this should be sufficient.
There was a problem hiding this comment.
Internally we checked for language version like it was a module kind in a number of places. Is it just a matter of not wanting to make it able to be specified explicitly?
There was a problem hiding this comment.
we do not support target: es5 with module: es6, so why allow it and add additional burden on users.
--target es6 with no --module is implied to be es6, if another --module tareget is specified, emit should use that.
There was a problem hiding this comment.
Alright, so I'm just removing it as an option for the --module command line flag.
|
@mhegazy anything else that I would need to look into for this? I looked through the diagnostic messages to see if we had any other features which were marked as ES6 dependent but were really ES6 module dependent, such as |
| @@ -13379,9 +13379,9 @@ namespace ts { | |||
| } | |||
| } | |||
| else { | |||
| if (languageVersion >= ScriptTarget.ES6 && !isInAmbientContext(node)) { | |||
| if (compilerOptions.module === ModuleKind.ES6 && !isInAmbientContext(node)) { | |||
There was a problem hiding this comment.
(languageVersion >= ScriptTarget.ES6 && !compilerOptions.module), or set that at the top of the file such as language version
There was a problem hiding this comment.
if you are adding an "es6" to module kind.
var modulekind = compilerOptions.module ? compilerOptions.module : compilerOptions.target === LaguageTaret.ES6 ? ModuleKind.ES6 : ModuleKind.None.
| @@ -44,6 +44,7 @@ namespace ts { | |||
|
|
|||
| let compilerOptions = host.getCompilerOptions(); | |||
| let languageVersion = compilerOptions.target || ScriptTarget.ES3; | |||
| var modulekind = compilerOptions.module ? compilerOptions.module : languageVersion === ScriptTarget.ES6 ? ModuleKind.ES6 : ModuleKind.None; | |||
There was a problem hiding this comment.
Perhaps it would be better to remove ModuleKind.ES6, as it could be confusing to API consumers, and instead use this here:
let moduleKind = compilerOptions.module || ModuleKind.None;
let nativeModules = languageVersion >= ScriptTarget.ES6 && moduleKind === ModuleKind.None;Then use the nativeModules boolean variable when checking below, or merely inline the expression used to compute it.
There was a problem hiding this comment.
Perhaps it would be better to remove ModuleKind.ES6, as it could be confusing to API consumers, and instead use this here
Please no. Not having ES6 as a module kind is all kinds of not forward compatible and causes odd checks which depend on there not being module kinds alongside specific language versions. Going forward, when we implement #4813 those forms of exports will not be ES6 compatible module emit, but can be compiled to ES6 compatible module emit. In effect, when we add an ES7 target and ES7 compatible module emit feature, we're going to need an ES6 module kind (and, to be consistent, hopefully an ES7 module kind!). Additionally, not having an ES6 module kind makes specifying that you want to emit an ES6 module style bundle for #4754 awkward. (Do I target ES6 but not specify module output type at all? That's oddly inconsistent.)
--module is one of our few variable feature flags at present, it's time we start treating it as such. ES6 is a kind of module emit. We should treat it as such. The module format is part of the ES6 spec, true, but nothing inherently ties the feature to the ES6 target only. Right now it seems nonsensical to ask for ES6 module syntax with a target below ES6 (since we know of 0 runtimes which support ES6 modules and yet support no other ES6 features), but we certainly could emit it.
IMO, having an ES6 module kind simplifies our API. When a module kind is set, it now controls the emit for the module format. Full stop.
|
👍 |
|
can you refresh. |
|
@mhegazy refreshed & retested. |
|
Ping @mhegazy: do we want to merge this? |
|
@rbuckton said he was looking for this, so I'm interpreting your 👍 as an lgtm now. |
Support modules when targeting ES6 and an ES6 ModuleKind
|
👍 |
|
@weswigham -- Thanks for this work. This unblocks a much better integration story between TypeScript and Ember-CLI 👍 |
|
very good job. |
#4806 and a part of #4389 refer to this change - people have been wanting to be able to disable emitting es6 module syntax when targeting es6 for awhile, and this allows them to do just that. While this could wait until we re-architect our emit for even more granular emit, even with our current emitter this wasn't a bad change to make.
Feelings about it?