improve module loading interoperability for babel #3586

vvakame · 2015-06-21T07:24:39Z

implementation for #2719

teppeis · 2015-06-21T17:14:21Z

GJ! 👍

teppeis · 2015-06-21T17:22:50Z

src/compiler/emitter.ts

@@ -2709,6 +2709,17 @@ var __param = (this && this.__param) || function (paramIndex, decorator) {
                    }
                    else {
                        if (node.flags & NodeFlags.Default) {
+                            if (compilerOptions.module === ModuleKind.CommonJS || compilerOptions.module === ModuleKind.AMD || compilerOptions.module === ModuleKind.UMD) {
+                                write("Object.defineProperty(exports, \"__esModule\", {");


Object.defineProperty is NOT available in ES3.
If compile target is ES3, you may not use it.
Just exports.__esModule = true; like babel's loose mode.

If you want to emulate this automatically, putting the define property behind a languageVersion >= ScriptTarget.ES5 and adding a write("exports.__esModule = true"); for es3 would work.

thanks! applied.

mhegazy · 2015-06-24T00:33:10Z

src/compiler/emitter.ts

+                            if (compilerOptions.module === ModuleKind.CommonJS || compilerOptions.module === ModuleKind.AMD || compilerOptions.module === ModuleKind.UMD) {
+                                if (languageVersion >= ScriptTarget.ES5) {
+                                    write("Object.defineProperty(exports, \"__esModule\", {");
+                                    writeLine();


nit: can we put it on one line. i.e. loose the indent and the new line.

vvakame · 2015-07-03T14:13:35Z

how is it?

RyanCavanaugh · 2015-07-06T18:57:59Z

Ping @mhegazy

mhegazy · 2015-07-06T19:05:52Z

Sorry for the delay. i had a comment that i wanted to fix as i merge the review, (!hasProperty(currentSourceFile.identifiers, "___esModule" is a bit too aggressive, as a local var with the name "__esModule" would trigger this behavior. though uncommon, i think we should only look for exported properties. i will take care of it, and hopefully get this change in today. sorry for the delay.

JsonFreeman · 2015-07-06T20:28:11Z

src/compiler/emitter.ts

@@ -2709,9 +2725,11 @@ var __param = (this && this.__param) || function (paramIndex, decorator) {
                    }
                    else {
                        if (node.flags & NodeFlags.Default) {
+                            emitEs6ExportDefaultCompat();


It was tricky to convince myself that this would not get called in ES6. To do so, I had to look at the calls to emitExportMemberAssignment. I found two:

emitClassLikeDeclarationBelowES6 (this one was obviously below ES6)

emitSignatureAndBody (this one was not obvious)

The call from emitSignatureAndBody is guarded by !isES6ExportedDeclaration. For isES6ExportedDeclaration to be false, any of its 3 disjuncts could be false. We want the ES6 one to be false, so let's trying to prove the other 2 true.

!!(node.flags & NodeFlags.Export): This is true because of line 2707

node.parent.kind === SyntaxKind.SourceFile: This is true by the rules of our language. We only allow export default at a source file level, or an ambient external module. We don't emit javascript for an ambient external module, so in legal code, it must be in a source file.

I think the following would help with static reasoning. Take the above check node.parent === currentSourceFile and guard both the System and the default cases with that. Possibly also change it to node.parent.kind === SyntaxKind.SourceFile to make it even clearer.

vvakame · 2015-07-12T11:45:56Z

@mhegazy @JsonFreeman thanks for reviewing. I make a new commit. is it reflect your opinions?

mhegazy · 2015-07-13T21:15:26Z

👍

vvakame · 2015-07-15T15:44:58Z

@JsonFreeman I want to assert calling context of emitEs6ExportDefaultCompat.
I think read function code is easier than reading context.

JsonFreeman · 2015-07-15T17:21:33Z

Can you change the conditions to Debug.assert calls then?

vvakame · 2015-07-15T18:06:22Z

@JsonFreeman sure!

JsonFreeman · 2015-07-15T19:06:20Z

src/compiler/emitter.ts

@@ -3012,6 +3012,26 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
                return result;
            }

+            function emitEs6ExportDefaultCompat(node: Node) {
+                if (node.parent.kind === SyntaxKind.SourceFile) {


Should this be an assert too?

@JsonFreeman I think this line is required. if this line changes into Debug.assert, we got a assertion error in wrong context.

3 failing 1) compiler tests for tests/cases/compiler/moduleElementsInWrongContext.ts "before all" hook: Error: Debug Failure. False expression: at Object.assert (eval at <anonymous> (built/local/run.js:52497:13), <anonymous>:1533:23) 2) compiler tests for tests/cases/compiler/moduleElementsInWrongContext2.ts "before all" hook: Error: Debug Failure. False expression: at Object.assert (eval at <anonymous> (built/local/run.js:52497:13), <anonymous>:1533:23) 3) compiler tests for tests/cases/compiler/moduleElementsInWrongContext3.ts "before all" hook: Error: Debug Failure. False expression: at Object.assert (eval at <anonymous> (built/local/run.js:52497:13), <anonymous>:1533:23)

vvakame · 2015-07-21T06:07:53Z

@JsonFreeman 💭

JsonFreeman · 2015-07-21T06:52:23Z

Sorry @vvakame, I did not see... This is good to go 👍 Thanks!

improve module loading interoperability for babel

vvakame · 2015-07-21T10:37:30Z

✨ 😸 ✨

mhegazy · 2015-07-21T21:05:58Z

thanks @vvakame !

improve module loading interoperability for babel

54a4e9e

msftclas added the cla-already-signed label Jun 21, 2015

teppeis reviewed Jun 21, 2015
View reviewed changes

do not use Object.defineProperty in es3 target

3aba5aa

mhegazy reviewed Jun 24, 2015
View reviewed changes

PR feedback

e9d590f

JsonFreeman reviewed Jul 6, 2015
View reviewed changes

vvakame added 2 commits July 11, 2015 18:10

Merge branch 'master' into addEsModule

75466e6

PR feedback

86d106a

PR feedback

a512e9e

JsonFreeman reviewed Jul 15, 2015
View reviewed changes

JsonFreeman added a commit that referenced this pull request Jul 21, 2015

Merge pull request #3586 from vvakame/addEsModule

4e0acc9

improve module loading interoperability for babel

JsonFreeman merged commit 4e0acc9 into microsoft:master Jul 21, 2015

vvakame deleted the addEsModule branch July 21, 2015 10:36

teppeis mentioned this pull request Jul 23, 2015

ES6 Modules default exports interop with CommonJS #2719

Closed

basarat mentioned this pull request Aug 16, 2015

React ES6 import issue DefinitelyTyped/DefinitelyTyped#5128

Closed

AJamesPhillips mentioned this pull request Apr 25, 2017

Add type definition for reduce-reducers DefinitelyTyped/DefinitelyTyped#14790

Merged

11 tasks

sansthesis mentioned this pull request Sep 7, 2017

Make let default work even better rescript-lang/rescript-compiler#1987

Closed

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve module loading interoperability for babel #3586

improve module loading interoperability for babel #3586

vvakame commented Jun 21, 2015

teppeis commented Jun 21, 2015

teppeis Jun 21, 2015

RichiCoder1 Jun 21, 2015

vvakame Jun 22, 2015

mhegazy Jun 24, 2015

vvakame Jun 26, 2015

vvakame commented Jul 3, 2015

RyanCavanaugh commented Jul 6, 2015

mhegazy commented Jul 6, 2015

JsonFreeman Jul 6, 2015

vvakame commented Jul 12, 2015

mhegazy commented Jul 13, 2015

vvakame commented Jul 15, 2015

JsonFreeman commented Jul 15, 2015

vvakame commented Jul 15, 2015

JsonFreeman Jul 15, 2015

vvakame Jul 16, 2015

vvakame commented Jul 21, 2015

JsonFreeman commented Jul 21, 2015

vvakame commented Jul 21, 2015

mhegazy commented Jul 21, 2015

improve module loading interoperability for babel #3586

improve module loading interoperability for babel #3586

Conversation

vvakame commented Jun 21, 2015

teppeis commented Jun 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvakame commented Jul 3, 2015

RyanCavanaugh commented Jul 6, 2015

mhegazy commented Jul 6, 2015

Choose a reason for hiding this comment

vvakame commented Jul 12, 2015

mhegazy commented Jul 13, 2015

vvakame commented Jul 15, 2015

JsonFreeman commented Jul 15, 2015

vvakame commented Jul 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvakame commented Jul 21, 2015

JsonFreeman commented Jul 21, 2015

vvakame commented Jul 21, 2015

mhegazy commented Jul 21, 2015