[Fix 13709] - Emit __esmodule #13871

yuit · 2017-02-03T19:58:44Z

Fix #13709. It is a big change because of the baselines. The meat of the change is in "module/modules.ts"

Couple question:

What should happen when user already defined "__esmodule"? Currently, we will not emit our version of "__esmodule". Babel gives an error for using such variable name. We will issue an error
Should we emit "__esmodule" for export import x = require ...? Yes

…port

mhegazy · 2017-02-04T00:35:35Z

src/compiler/transformers/module/module.ts

@@ -378,6 +383,10 @@ namespace ts {
            // Append the 'export =' statement if provided.
            addExportEqualsIfNeeded(statements, /*emitAsReturn*/ true);

+            if (shouldAppendUnderscoreUnderscoreEsModule) {


why do we set it if we have export = ?

We didn't set shouldAppendUnderscoreUnderscoreEsModule here. This is after we have passed through transform module already and we just check if anyone set the flag

DanielRosenwasser · 2017-02-04T00:34:34Z

src/compiler/transformers/module/module.ts

@@ -657,6 +666,8 @@ namespace ts {
            }

            const generatedName = getGeneratedNameForNode(node);
+            shouldAppendUnderscoreUnderscoreEsModule = true;


shouldAppendEsModuleMarker

DanielRosenwasser · 2017-02-04T00:35:13Z

src/compiler/transformers/module/module.ts

+
+            const parseTreeNode = getParseTreeNode(node);
+            if (!shouldAppendUnderscoreUnderscoreEsModule) {
+                // class declaration get down-level transformed to be variable statement


declaration -> declarations
statement -> statements

…rt import

rbuckton

I think we should make any top-level exported declaration of __esModule an error.

rbuckton · 2017-02-04T02:26:43Z

src/compiler/transformers/module/module.ts

@@ -820,6 +831,14 @@ namespace ts {
            let statements: Statement[];
            let variables: VariableDeclaration[];
            let expressions: Expression[];
+
+            const parseTreeNode = getParseTreeNode(node);


Why do you need this check? Wouldn't setting shouldAppendEsModuleMarker to true in createExportExpression be sufficient? Basically, if I export anything other than via an export=, then I need to append __esModule. Would that be correct? Alternatively, always add it if you don't have an export= (e.g. currentModuleInfo.exportEquals === false).

So just setting on in createExportExpression won't be enough because only VariableStatement with bindingPattern will call into the function.

Also we can't just check for currentModuleInfo.exportEquals === false
because we will be issue extra __esModule even though it doesn't need one

For example:

export enum SignatureFlags { None = 0, IsIndexer = 1, IsStringIndexer = 1 << 1, IsNumberIndexer = 1 << 2, }

will get __esModule as well

Why wouldn't we emit __esModule here? Perhaps I'm not understanding when we do/don't want to emit this marker.

@rbuckton After some thought, I think we should emit one here. Though in the following case, I don't think we should. By checking !currentModuleIndo.exportEqual, we will emit the marker still.

import * as Foo from "Foo"

mhegazy · 2017-02-06T23:22:21Z

src/compiler/checker.ts

@@ -21921,6 +21925,22 @@ namespace ts {
            return checkLetConstNames && checkGrammarNameInLetOrConstDeclarations(node.name);
        }

+        function checkESModuleMarker(name: Identifier | BindingPattern): boolean {
+            if (name.kind === SyntaxKind.Identifier) {
+                if (unescapeIdentifier(name.text) === "__esModule") {


The error should not be shown in ambient contexts and if --noEmit is set.

as we discuss, module kind of system should not show an error as well

DanielRosenwasser · 2017-02-07T04:46:11Z

src/compiler/diagnosticMessages.json

@@ -671,6 +671,10 @@
        "category": "Error",
        "code": 1215
    },
+    "Identifier expected. '__esModule' is reserved as a marker for transpiled ES2015 module.": {


I think "Identifier expected" is a confusing bit of information that probably shouldn't be reported here.

I think this would be a better message:

'__esModule' is reserved as a top-level export when transforming ECMAScript modules.

I like the new message!. But Identifier expected should be there I think ( similar to how we report similar error for reserved word)

I don't think the Identifier expected is useful in any way. __esModule is an identifier. 😄

# Conflicts: # src/compiler/transformers/module/module.ts

yuit · 2017-02-07T23:00:08Z

🔔 @DanielRosenwasser @mhegazy @rbuckton

Jessidhia · 2017-02-08T23:29:19Z

src/compiler/transformers/module/module.ts

@@ -88,11 +88,14 @@ namespace ts {
            addRange(statements, endLexicalEnvironment());
            addExportEqualsIfNeeded(statements, /*emitAsReturn*/ false);

+            if (!currentModuleInfo.exportEquals) {
+                append(statements, createUnderscoreUnderscoreESModule());


Not really familiar with TS codebase to know if this is correct and/or easily fixable, but the __esModule marker should be created as soon as possible. It needs to be visible from circular imports.

@Kovensky do you mean __esModule has to be emitted as a first statement in the output to prevent circular imports? Could you elaborate on your comments? thanks!

@yuit not to prevent circular imports, but rather to enable them. Consider:

// a.ts import b from './b'; export default function(x) { return x ? b(false) : 1; } // b.ts import a from './a'; export default function(x) { return x ? a(false) : 2; }

Both a.ts and b.ts should be recognized as an ES module as early as possible, before the first require() is called in either module.

Consider the following scenario, where we emit __esModule at the end:

// a.js var b_1 = require("./b"); exports.default = function (x) { return x ? b_1(false) : 1; }; Object.defineProperty(exports, "__esModule", { value: true }); // b.js var a_1 = require("./a"); exports.default = function (x) { return x ? a_1(false) : 2; } Object.defineProperty(exports, "__esModule", { value: true });

Assuming a.js is loaded first:

A module object will be created for a.js and stored in Node's module cache. At this point, Node (or rather, some future version) does not know whether it is an ES module or a CJS module.

The a.js module body begins to execute and a.js invokes a require() for b.js.

A module object will be created for b.js and stored in Node's module cache.

The b.js module body begins to execute the module body and invokes a require() for a.js.

Node cannot see the __esModule marker on the exports object for a.js and doesn't know is an ES module, and therefore cannot provide the right answer to b.js.

Now consider the same scenario, with the __esModule marker at the beginning:

// a.js Object.defineProperty(exports, "__esModule", { value: true }); var b_1 = require("./b"); exports.default = function (x) { return x ? b_1(false) : 1; }; // b.js Object.defineProperty(exports, "__esModule", { value: true }); var a_1 = require("./a"); exports.default = function (x) { return x ? a_1(false) : 2; }

Assuming a.js is loaded first:

A module object will be created for a.js and stored in Node's module cache. At this point, Node (or rather, some future version) does not know whether it is an ES module or a CJS module.

The a.js module body begins to execute and a.js invokes a require() for b.js.

A module object will be created for b.js and stored in Node's module cache.

The b.js module body begins to execute the module body and invokes a require() for a.js.

Node can see the __esModule marker on the exports object for a.js and knows it can treat it as an ES module.

However, export = cannot be used circularly to begin with so this whole topic is possibly moot.

@DanielRosenwasser, is this an accurate description of the mechanics?

@rbuckton thank you for the thorough explanation! To be safe, we should then emit __esModule first. this seems to be safer than emit at the end. However, it will be after some helpers (as those are added by previous transformer)

…ript into master-fix13709

yuit · 2017-02-13T20:32:42Z

talk offline with @rbuckton, he gives a green light

popomore · 2017-02-23T08:19:16Z

I'm requiring a ts file that has compiled from a commonjs file, how can I check is it export defualt?

Is it simply checking whether contain default property?

Compile such as babel, typescript will compile es module always containing `__esModule`, we should do right things. Typescript 2.2 fix this bug in microsoft/TypeScript#13871

mhegazy · 2017-02-23T19:42:52Z

I'm requiring a ts file that has compiled from a commonjs file, how can I check is it export defualt?
Is it simply checking whether contain default property?

yes. TypeScript does not change any thing about your default export. if the module has a property called "default" then it has a default export.

Kanchalai Tanglertsampan added 5 commits February 1, 2017 16:15

Update baselines from a-c

8372a1d

Emit __esModule when emitting ES6 Module

2ff2850

Check original variable statement node to check if it contains es6 ex…

a8db366

…port

Update baselines

3f57f01

Update baselines (2)

4331de3

yuit requested a review from rbuckton February 3, 2017 19:58

msftclas added the cla-not-required label Feb 3, 2017

yuit requested a review from DanielRosenwasser February 3, 2017 19:59

mhegazy reviewed Feb 4, 2017

View reviewed changes

DanielRosenwasser reviewed Feb 4, 2017

View reviewed changes

mhegazy approved these changes Feb 4, 2017

View reviewed changes

Kanchalai Tanglertsampan added 2 commits February 3, 2017 17:38

Give a better name for the boolean variable; emit __esmodule for expo…

c0ac84f

…rt import

Update baselines (3)

535a001

rbuckton reviewed Feb 4, 2017

View reviewed changes

Kanchalai Tanglertsampan added 3 commits February 6, 2017 11:08

Error when users define "__esModule"

a34834c

Update sourcemap

444a55f

Fix linting

11cac43

mhegazy suggested changes Feb 6, 2017

View reviewed changes

Yui T added 2 commits February 6, 2017 20:07

Only report an error in non ambient context and with no noEmit flag

41f6c5e

Add test and baseline

d737104

DanielRosenwasser requested changes Feb 7, 2017

View reviewed changes

Kanchalai Tanglertsampan added 5 commits February 7, 2017 07:18

Address code review: update error message

b6c97de

Merge branch 'master' into master-fix13709

eedc465

# Conflicts: # src/compiler/transformers/module/module.ts

Always emit "__esModule" unless there is export =

bc68604

Update baselines

6621096

Allow export "__esModule" in system

229ba43

Jessidhia reviewed Feb 8, 2017

View reviewed changes

Kanchalai Tanglertsampan added 2 commits February 10, 2017 12:46

Emit "__esModule" before other statments

15935ec

Emit "__esModule" before other statments

dc1ac13

Kanchalai Tanglertsampan added 2 commits February 10, 2017 12:48

Merge branch 'master-fix13709' of https://github.com/Microsoft/TypeSc…

b250acd

…ript into master-fix13709

Update unittests

1243e11

yuit merged commit bc1058e into master Feb 13, 2017

yuit deleted the master-fix13709 branch February 13, 2017 20:32

popomore mentioned this pull request Feb 23, 2017

fix: improve require es module eggjs/core#61

Merged

4 tasks

microsoft locked and limited conversation to collaborators Jun 19, 2018

[Fix 13709] - Emit __esmodule #13871

[Fix 13709] - Emit __esmodule #13871

Uh oh!

Conversation

yuit commented Feb 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuit Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuit commented Feb 7, 2017

Uh oh!

Jessidhia Feb 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuit Feb 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbuckton Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuit Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuit commented Feb 13, 2017

Uh oh!

popomore commented Feb 23, 2017

Uh oh!

mhegazy commented Feb 23, 2017

Uh oh!

Uh oh!

yuit commented Feb 3, 2017 •

edited

Loading

DanielRosenwasser Feb 7, 2017 •

edited

Loading

yuit Feb 7, 2017 •

edited

Loading

Jessidhia Feb 8, 2017 •

edited

Loading

yuit Feb 9, 2017 •

edited

Loading

rbuckton Feb 10, 2017 •

edited

Loading

yuit Feb 10, 2017 •

edited

Loading