From 81fef20db9e945757631360febf6b1439069d351 Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Wed, 1 Nov 2017 18:45:10 -0500
Subject: [PATCH 01/13] loader,docs,test: set named exports based on keys from
 `module.exports`

I know a lot of discussion went into the original decision on how mjs
would handle cjs modules but after using the system for a while, and
talking with a lot of other people in the community, it just seems
like the expected behavior and the wanted behavior is to export named
based on the keys. This PR implements that in what is hopefully a
performant enough solution, although that shouldn't be too much of a
problem since this code only runs during initial module loading.

This implementation remains safe with regard to named exports that are
also reserved keywords such as `class` or `delete`.

Refs: https://github.com/nodejs/node-eps/issues/57
---
 doc/api/esm.md                        | 10 ++++++--
 lib/internal/loader/ModuleRequest.js  | 33 ++++++++++++++++-----------
 test/es-module/test-esm-namespace.mjs |  6 +++--
 3 files changed, 32 insertions(+), 17 deletions(-)

diff --git a/doc/api/esm.md b/doc/api/esm.md
index d8143da378f7..f3bb49569414 100644
--- a/doc/api/esm.md
+++ b/doc/api/esm.md
@@ -83,8 +83,9 @@ All CommonJS, JSON, and C++ modules can be used with `import`.
 Modules loaded this way will only be loaded once, even if their query
 or fragment string differs between `import` statements.
 
-When loaded via `import` these modules will provide a single `default` export
-representing the value of `module.exports` at the time they finished evaluating.
+When loaded via `import` these modules will provide a `default` export
+representing the value of `module.exports` at the time they finished evaluating,
+and named exports for each key of `module.exports`.
 
 ```js
 import fs from 'fs';
@@ -97,6 +98,11 @@ fs.readFile('./foo.txt', (err, body) => {
 });
 ```
 
+```js
+import { readFileSync } from 'fs';
+console.log(readFileSync('./foo.txt').toString());
+```
+
 ## Loader hooks
 
 <!-- type=misc -->
diff --git a/lib/internal/loader/ModuleRequest.js b/lib/internal/loader/ModuleRequest.js
index 72f3dd3ee570..50ce90bb73ea 100644
--- a/lib/internal/loader/ModuleRequest.js
+++ b/lib/internal/loader/ModuleRequest.js
@@ -36,31 +36,38 @@ loaders.set('esm', async (url) => {
 
 // Strategy for loading a node-style CommonJS module
 loaders.set('cjs', async (url) => {
-  return createDynamicModule(['default'], url, (reflect) => {
-    debug(`Loading CJSModule ${url}`);
-    const CJSModule = require('module');
-    const pathname = internalURLModule.getPathFromURL(new URL(url));
-    CJSModule._load(pathname);
+  debug(`Loading CJSModule ${url}`);
+  const CJSModule = require('module');
+  const pathname = internalURLModule.getPathFromURL(new URL(url));
+  const exports = CJSModule._load(pathname);
+  const keys = Object.keys(exports);
+  return createDynamicModule(['default', ...keys], url, (reflect) => {
+    reflect.exports.default.set(exports);
+    for (const key of keys) reflect.exports[key].set(exports[key]);
   });
 });
 
 // Strategy for loading a node builtin CommonJS module that isn't
 // through normal resolution
 loaders.set('builtin', async (url) => {
-  return createDynamicModule(['default'], url, (reflect) => {
-    debug(`Loading BuiltinModule ${url}`);
-    const exports = NativeModule.require(url.substr(5));
+  debug(`Loading BuiltinModule ${url}`);
+  const exports = NativeModule.require(url.substr(5));
+  const keys = Object.keys(exports);
+  return createDynamicModule(['default', ...keys], url, (reflect) => {
     reflect.exports.default.set(exports);
+    for (const key of keys) reflect.exports[key].set(exports[key]);
   });
 });
 
 loaders.set('addon', async (url) => {
-  const ctx = createDynamicModule(['default'], url, (reflect) => {
-    debug(`Loading NativeModule ${url}`);
-    const module = { exports: {} };
-    const pathname = internalURLModule.getPathFromURL(new URL(url));
-    process.dlopen(module, _makeLong(pathname));
+  debug(`Loading NativeModule ${url}`);
+  const module = { exports: {} };
+  const pathname = internalURLModule.getPathFromURL(new URL(url));
+  process.dlopen(module, _makeLong(pathname));
+  const keys = Object.keys(module.exports);
+  const ctx = createDynamicModule(['default', ...keys], url, (reflect) => {
     reflect.exports.default.set(module.exports);
+    for (const key of keys) reflect.exports[key].set(module.exports[key]);
   });
   return ctx;
 });
diff --git a/test/es-module/test-esm-namespace.mjs b/test/es-module/test-esm-namespace.mjs
index 72b7fed4b33d..d29d6d3ee8af 100644
--- a/test/es-module/test-esm-namespace.mjs
+++ b/test/es-module/test-esm-namespace.mjs
@@ -1,7 +1,9 @@
 // Flags: --experimental-modules
 /* eslint-disable required-modules */
 
-import * as fs from 'fs';
 import assert from 'assert';
+import fs, { readFile } from 'fs';
 
-assert.deepStrictEqual(Object.keys(fs), ['default']);
+assert(fs);
+assert(fs.readFile);
+assert(readFile);

From d3647ddd4cd5773cfe5159e42412e1624d587e49 Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Wed, 1 Nov 2017 20:36:38 -0500
Subject: [PATCH 02/13] add test for exporting reserved keywords from cjs from
 esm

---
 test/es-module/test-reserved-keywords.mjs            | 8 ++++++++
 test/fixtures/es-module-loaders/reserved-keywords.js | 5 +++++
 2 files changed, 13 insertions(+)
 create mode 100644 test/es-module/test-reserved-keywords.mjs
 create mode 100644 test/fixtures/es-module-loaders/reserved-keywords.js

diff --git a/test/es-module/test-reserved-keywords.mjs b/test/es-module/test-reserved-keywords.mjs
new file mode 100644
index 000000000000..d4fe62f47ce9
--- /dev/null
+++ b/test/es-module/test-reserved-keywords.mjs
@@ -0,0 +1,8 @@
+// Flags: --experimental-modules
+/* eslint-disable required-modules */
+
+import assert from 'assert';
+import { delete as d } from
+  '../fixtures/es-module-loaders/reserved-keywords.js';
+
+assert(d);
diff --git a/test/fixtures/es-module-loaders/reserved-keywords.js b/test/fixtures/es-module-loaders/reserved-keywords.js
new file mode 100644
index 000000000000..3fc03e9d8739
--- /dev/null
+++ b/test/fixtures/es-module-loaders/reserved-keywords.js
@@ -0,0 +1,5 @@
+module.exports = {
+  enum: 'enum',
+  class: 'class',
+  delete: 'delete',
+};

From e8478e28f5aeaf451f3c921ce1dfe452fbeebc58 Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Thu, 2 Nov 2017 11:06:53 -0500
Subject: [PATCH 03/13] Slight change of plan with loading (use well-defined
 __esModule)

A very very large portion of the js community (babel/typescript)
use `__esModule` to define an es module in CJS.
---
 doc/api/esm.md                                | 26 +++++++++++++-----
 lib/internal/loader/ModuleRequest.js          | 27 +++++++++++--------
 test/es-module/test-esm-namespace.mjs         |  7 ++++-
 test/es-module/test-reserved-keywords.mjs     |  4 +--
 .../es-module-loaders/cjs-to-es-namespace.js  |  4 +++
 .../es-module-loaders/reserved-keywords.js    |  1 +
 6 files changed, 48 insertions(+), 21 deletions(-)
 create mode 100644 test/fixtures/es-module-loaders/cjs-to-es-namespace.js

diff --git a/doc/api/esm.md b/doc/api/esm.md
index f3bb49569414..4b7a0efe2dfe 100644
--- a/doc/api/esm.md
+++ b/doc/api/esm.md
@@ -83,13 +83,21 @@ All CommonJS, JSON, and C++ modules can be used with `import`.
 Modules loaded this way will only be loaded once, even if their query
 or fragment string differs between `import` statements.
 
-When loaded via `import` these modules will provide a `default` export
-representing the value of `module.exports` at the time they finished evaluating,
-and named exports for each key of `module.exports`.
+CommonJS modules, when imported, will be handled in one of two ways. By default
+they will provide a single `default` export representing the value of
+`module.exports` at the time they finish evaluating. However, they may also
+provide `__esModule` as per
+[babel spec](https://babeljs.io/docs/plugins/transform-es2015-modules-commonjs)
+to use named exports, representing each enumerable key of `module.exports` at
+the time they finish evaluating.
+In both cases, this should be thought of  like a "snapshot" of the exports at
+the time of importing; asynchronously modifying `module.exports` will not
+affect the values of the exports. Builtin libraries such as `fs` are provided
+with named exports as if they were using `__esModule`
 
 ```js
-import fs from 'fs';
-fs.readFile('./foo.txt', (err, body) => {
+import { readFile } from 'fs';
+readFile('./foo.txt', (err, body) => {
   if (err) {
     console.error(err);
   } else {
@@ -99,8 +107,12 @@ fs.readFile('./foo.txt', (err, body) => {
 ```
 
 ```js
-import { readFileSync } from 'fs';
-console.log(readFileSync('./foo.txt').toString());
+// main.mjs
+import { part } from './other.js';
+
+// other.js
+exports.part = () => {};
+exports.__esModule = true;
 ```
 
 ## Loader hooks
diff --git a/lib/internal/loader/ModuleRequest.js b/lib/internal/loader/ModuleRequest.js
index 50ce90bb73ea..17b816d75093 100644
--- a/lib/internal/loader/ModuleRequest.js
+++ b/lib/internal/loader/ModuleRequest.js
@@ -35,15 +35,22 @@ loaders.set('esm', async (url) => {
 });
 
 // Strategy for loading a node-style CommonJS module
+// Uses babel and typescript style __esModule property for
+// attaching named imports due to the possiblity of `module.exports.default`
 loaders.set('cjs', async (url) => {
   debug(`Loading CJSModule ${url}`);
   const CJSModule = require('module');
   const pathname = internalURLModule.getPathFromURL(new URL(url));
   const exports = CJSModule._load(pathname);
-  const keys = Object.keys(exports);
-  return createDynamicModule(['default', ...keys], url, (reflect) => {
-    reflect.exports.default.set(exports);
-    for (const key of keys) reflect.exports[key].set(exports[key]);
+  const es = !!exports.__esModule;
+  const keys = es ? Object.keys(exports) : ['default'];
+  return createDynamicModule(keys, url, (reflect) => {
+    if (es) {
+      for (const key of keys)
+        reflect.exports[key].set(exports[key]);
+    } else {
+      reflect.exports.default.set(exports);
+    }
   });
 });
 
@@ -60,14 +67,12 @@ loaders.set('builtin', async (url) => {
 });
 
 loaders.set('addon', async (url) => {
-  debug(`Loading NativeModule ${url}`);
-  const module = { exports: {} };
-  const pathname = internalURLModule.getPathFromURL(new URL(url));
-  process.dlopen(module, _makeLong(pathname));
-  const keys = Object.keys(module.exports);
-  const ctx = createDynamicModule(['default', ...keys], url, (reflect) => {
+  const ctx = createDynamicModule(['default'], url, (reflect) => {
+    debug(`Loading NativeModule ${url}`);
+    const module = { exports: {} };
+    const pathname = internalURLModule.getPathFromURL(new URL(url));
+    process.dlopen(module, _makeLong(pathname));
     reflect.exports.default.set(module.exports);
-    for (const key of keys) reflect.exports[key].set(module.exports[key]);
   });
   return ctx;
 });
diff --git a/test/es-module/test-esm-namespace.mjs b/test/es-module/test-esm-namespace.mjs
index d29d6d3ee8af..856fdfeebb6c 100644
--- a/test/es-module/test-esm-namespace.mjs
+++ b/test/es-module/test-esm-namespace.mjs
@@ -3,7 +3,12 @@
 
 import assert from 'assert';
 import fs, { readFile } from 'fs';
+import main, { named } from
+  '../fixtures/es-module-loaders/cjs-to-es-namespace.js';
 
 assert(fs);
 assert(fs.readFile);
-assert(readFile);
+assert.strictEqual(fs.readFile, readFile);
+
+assert.strictEqual(main, 1);
+assert.strictEqual(named, true);
diff --git a/test/es-module/test-reserved-keywords.mjs b/test/es-module/test-reserved-keywords.mjs
index d4fe62f47ce9..02eb5733c960 100644
--- a/test/es-module/test-reserved-keywords.mjs
+++ b/test/es-module/test-reserved-keywords.mjs
@@ -2,7 +2,7 @@
 /* eslint-disable required-modules */
 
 import assert from 'assert';
-import { delete as d } from
+import { enum as e } from
   '../fixtures/es-module-loaders/reserved-keywords.js';
 
-assert(d);
+assert(e);
diff --git a/test/fixtures/es-module-loaders/cjs-to-es-namespace.js b/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
new file mode 100644
index 000000000000..f1aaa75e6fb8
--- /dev/null
+++ b/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
@@ -0,0 +1,4 @@
+exports.named = true;
+exports.default=  1;
+
+Object.defineProperty(exports, '__esModule', { value: true });
diff --git a/test/fixtures/es-module-loaders/reserved-keywords.js b/test/fixtures/es-module-loaders/reserved-keywords.js
index 3fc03e9d8739..4d043d30e657 100644
--- a/test/fixtures/es-module-loaders/reserved-keywords.js
+++ b/test/fixtures/es-module-loaders/reserved-keywords.js
@@ -2,4 +2,5 @@ module.exports = {
   enum: 'enum',
   class: 'class',
   delete: 'delete',
+  __esModule: true,
 };

From 116b470858b04cdaa1255eea4776079e8c42e4ad Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Thu, 2 Nov 2017 12:49:05 -0500
Subject: [PATCH 04/13] Symbol('esModule') exported from `module` builtin

recommends @@esModule over __esModule
---
 doc/api/esm.md                                 | 18 +++++++++---------
 lib/internal/loader/ModuleRequest.js           |  4 +++-
 lib/module.js                                  |  2 ++
 .../es-module-loaders/cjs-to-es-namespace.js   |  6 ++++--
 .../es-module-loaders/reserved-keywords.js     |  4 +++-
 5 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/doc/api/esm.md b/doc/api/esm.md
index 4b7a0efe2dfe..b43feb2f544d 100644
--- a/doc/api/esm.md
+++ b/doc/api/esm.md
@@ -86,14 +86,13 @@ or fragment string differs between `import` statements.
 CommonJS modules, when imported, will be handled in one of two ways. By default
 they will provide a single `default` export representing the value of
 `module.exports` at the time they finish evaluating. However, they may also
-provide `__esModule` as per
-[babel spec](https://babeljs.io/docs/plugins/transform-es2015-modules-commonjs)
-to use named exports, representing each enumerable key of `module.exports` at
-the time they finish evaluating.
-In both cases, this should be thought of  like a "snapshot" of the exports at
-the time of importing; asynchronously modifying `module.exports` will not
-affect the values of the exports. Builtin libraries such as `fs` are provided
-with named exports as if they were using `__esModule`
+provide `@@esModule` or `__esModule` to use named exports, representing each
+enumerable key of `module.exports` at the time they finish evaluating.
+`@@esModule` is available as `require('module').esModule` and prefered over
+`__esModule` In both cases, this should be thought of  like a "snapshot" of
+the exports at the time of importing; asynchronously modifying `module.exports`
+will not affect the values of the exports. Builtin libraries are provided with
+named exports as if they were using `@@esModule`.
 
 ```js
 import { readFile } from 'fs';
@@ -111,8 +110,9 @@ readFile('./foo.txt', (err, body) => {
 import { part } from './other.js';
 
 // other.js
+import { esModule } from 'module';
 exports.part = () => {};
-exports.__esModule = true;
+exports[esModule] = true;
 ```
 
 ## Loader hooks
diff --git a/lib/internal/loader/ModuleRequest.js b/lib/internal/loader/ModuleRequest.js
index 17b816d75093..c991cf2d6328 100644
--- a/lib/internal/loader/ModuleRequest.js
+++ b/lib/internal/loader/ModuleRequest.js
@@ -19,6 +19,8 @@ const search = require('internal/loader/search');
 const asyncReadFile = require('util').promisify(require('fs').readFile);
 const debug = require('util').debuglog('esm');
 
+const esModuleSymbol = exports.esModuleSymbol = Symbol('esModule');
+
 const realpathCache = new Map();
 
 const loaders = new Map();
@@ -42,7 +44,7 @@ loaders.set('cjs', async (url) => {
   const CJSModule = require('module');
   const pathname = internalURLModule.getPathFromURL(new URL(url));
   const exports = CJSModule._load(pathname);
-  const es = !!exports.__esModule;
+  const es = Boolean(exports[esModuleSymbol] || exports.__esModule);
   const keys = es ? Object.keys(exports) : ['default'];
   return createDynamicModule(keys, url, (reflect) => {
     if (es) {
diff --git a/lib/module.js b/lib/module.js
index e418a1a3e084..a604eba7e85a 100644
--- a/lib/module.js
+++ b/lib/module.js
@@ -42,6 +42,7 @@ const errors = require('internal/errors');
 const Loader = require('internal/loader/Loader');
 const ModuleJob = require('internal/loader/ModuleJob');
 const { createDynamicModule } = require('internal/loader/ModuleWrap');
+const { esModuleSymbol } = require('internal/loader/ModuleRequest');
 let ESMLoader;
 
 function stat(filename) {
@@ -79,6 +80,7 @@ Module._pathCache = Object.create(null);
 Module._extensions = Object.create(null);
 var modulePaths = [];
 Module.globalPaths = [];
+Module.esModule = esModuleSymbol;
 
 Module.wrap = function(script) {
   return Module.wrapper[0] + script + Module.wrapper[1];
diff --git a/test/fixtures/es-module-loaders/cjs-to-es-namespace.js b/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
index f1aaa75e6fb8..196e2d4d2f77 100644
--- a/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
+++ b/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
@@ -1,4 +1,6 @@
+const { esModule } = require('module');
+
 exports.named = true;
-exports.default=  1;
+exports.default = 1;
 
-Object.defineProperty(exports, '__esModule', { value: true });
+exports[esModule] = true;
diff --git a/test/fixtures/es-module-loaders/reserved-keywords.js b/test/fixtures/es-module-loaders/reserved-keywords.js
index 4d043d30e657..8445140a8e0f 100644
--- a/test/fixtures/es-module-loaders/reserved-keywords.js
+++ b/test/fixtures/es-module-loaders/reserved-keywords.js
@@ -1,6 +1,8 @@
+const { esModule } = require('module');
+
 module.exports = {
   enum: 'enum',
   class: 'class',
   delete: 'delete',
-  __esModule: true,
+  [esModule]: true,
 };

From 1a53fd195ad58ff16e9c391fa1d9d3a8a3969e70 Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Thu, 2 Nov 2017 12:52:38 -0500
Subject: [PATCH 05/13] remove comment i forgot to remove

---
 lib/internal/loader/ModuleRequest.js | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/internal/loader/ModuleRequest.js b/lib/internal/loader/ModuleRequest.js
index c991cf2d6328..f9ab7bc7bef6 100644
--- a/lib/internal/loader/ModuleRequest.js
+++ b/lib/internal/loader/ModuleRequest.js
@@ -37,8 +37,6 @@ loaders.set('esm', async (url) => {
 });
 
 // Strategy for loading a node-style CommonJS module
-// Uses babel and typescript style __esModule property for
-// attaching named imports due to the possiblity of `module.exports.default`
 loaders.set('cjs', async (url) => {
   debug(`Loading CJSModule ${url}`);
   const CJSModule = require('module');

From ac2b4a1fb000a1337b96f192406a38632521b12f Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Thu, 2 Nov 2017 13:28:22 -0500
Subject: [PATCH 06/13] change up symbol usage, use for(;;), add tests

---
 doc/api/esm.md                                   | 16 +++++++---------
 lib/internal/loader/ModuleRequest.js             | 11 ++++++-----
 lib/module.js                                    |  2 --
 test/es-module/test-esm-cjs-esmodule.mjs         |  9 +++++++++
 test/fixtures/es-module-loaders/babel-to-esm.js  | 15 +++++++++++++++
 .../es-module-loaders/cjs-to-es-namespace.js     |  4 +---
 .../es-module-loaders/reserved-keywords.js       |  4 +---
 7 files changed, 39 insertions(+), 22 deletions(-)
 create mode 100644 test/es-module/test-esm-cjs-esmodule.mjs
 create mode 100644 test/fixtures/es-module-loaders/babel-to-esm.js

diff --git a/doc/api/esm.md b/doc/api/esm.md
index b43feb2f544d..bdbaf4747b62 100644
--- a/doc/api/esm.md
+++ b/doc/api/esm.md
@@ -86,13 +86,12 @@ or fragment string differs between `import` statements.
 CommonJS modules, when imported, will be handled in one of two ways. By default
 they will provide a single `default` export representing the value of
 `module.exports` at the time they finish evaluating. However, they may also
-provide `@@esModule` or `__esModule` to use named exports, representing each
-enumerable key of `module.exports` at the time they finish evaluating.
-`@@esModule` is available as `require('module').esModule` and prefered over
-`__esModule` In both cases, this should be thought of  like a "snapshot" of
-the exports at the time of importing; asynchronously modifying `module.exports`
-will not affect the values of the exports. Builtin libraries are provided with
-named exports as if they were using `@@esModule`.
+provide `@@esModuleInterop` or `__esModule` to use named exports, representing
+each enumerable key of `module.exports` at the time they finish evaluating.
+In both cases, this should be thought of  like a "snapshot" of the exports at
+the time of importing; asynchronously modifying `module.exports` will not
+affect the values of the exports. Builtin libraries are provided with named
+exports as if they were using `@@esModuleInterop`.
 
 ```js
 import { readFile } from 'fs';
@@ -110,9 +109,8 @@ readFile('./foo.txt', (err, body) => {
 import { part } from './other.js';
 
 // other.js
-import { esModule } from 'module';
 exports.part = () => {};
-exports[esModule] = true;
+exports[Symbol.for('esModuleInterop')] = true;
 ```
 
 ## Loader hooks
diff --git a/lib/internal/loader/ModuleRequest.js b/lib/internal/loader/ModuleRequest.js
index f9ab7bc7bef6..0ee9d8d3124f 100644
--- a/lib/internal/loader/ModuleRequest.js
+++ b/lib/internal/loader/ModuleRequest.js
@@ -19,7 +19,7 @@ const search = require('internal/loader/search');
 const asyncReadFile = require('util').promisify(require('fs').readFile);
 const debug = require('util').debuglog('esm');
 
-const esModuleSymbol = exports.esModuleSymbol = Symbol('esModule');
+const esModuleInterop = Symbol.for('esModuleInterop');
 
 const realpathCache = new Map();
 
@@ -42,12 +42,12 @@ loaders.set('cjs', async (url) => {
   const CJSModule = require('module');
   const pathname = internalURLModule.getPathFromURL(new URL(url));
   const exports = CJSModule._load(pathname);
-  const es = Boolean(exports[esModuleSymbol] || exports.__esModule);
+  const es = Boolean(exports[esModuleInterop] || exports.__esModule);
   const keys = es ? Object.keys(exports) : ['default'];
   return createDynamicModule(keys, url, (reflect) => {
     if (es) {
-      for (const key of keys)
-        reflect.exports[key].set(exports[key]);
+      for (let i = 0; i < keys.length; i++)
+        reflect.exports[keys[i]].set(exports[keys[i]]);
     } else {
       reflect.exports.default.set(exports);
     }
@@ -62,7 +62,8 @@ loaders.set('builtin', async (url) => {
   const keys = Object.keys(exports);
   return createDynamicModule(['default', ...keys], url, (reflect) => {
     reflect.exports.default.set(exports);
-    for (const key of keys) reflect.exports[key].set(exports[key]);
+    for (let i = 0; i < keys.length; i++)
+      reflect.exports[keys[i]].set(exports[keys[i]]);
   });
 });
 
diff --git a/lib/module.js b/lib/module.js
index a604eba7e85a..e418a1a3e084 100644
--- a/lib/module.js
+++ b/lib/module.js
@@ -42,7 +42,6 @@ const errors = require('internal/errors');
 const Loader = require('internal/loader/Loader');
 const ModuleJob = require('internal/loader/ModuleJob');
 const { createDynamicModule } = require('internal/loader/ModuleWrap');
-const { esModuleSymbol } = require('internal/loader/ModuleRequest');
 let ESMLoader;
 
 function stat(filename) {
@@ -80,7 +79,6 @@ Module._pathCache = Object.create(null);
 Module._extensions = Object.create(null);
 var modulePaths = [];
 Module.globalPaths = [];
-Module.esModule = esModuleSymbol;
 
 Module.wrap = function(script) {
   return Module.wrapper[0] + script + Module.wrapper[1];
diff --git a/test/es-module/test-esm-cjs-esmodule.mjs b/test/es-module/test-esm-cjs-esmodule.mjs
new file mode 100644
index 000000000000..1692f3505bd2
--- /dev/null
+++ b/test/es-module/test-esm-cjs-esmodule.mjs
@@ -0,0 +1,9 @@
+// Flags: --experimental-modules
+/* eslint-disable required-modules */
+
+import assert from 'assert';
+import eightyfour, { named as fourtytwo } from
+  '../fixtures/es-module-loaders/babel-to-esm.js';
+
+assert.strictEqual(eightyfour, 84);
+assert.strictEqual(fourtytwo, 42);
diff --git a/test/fixtures/es-module-loaders/babel-to-esm.js b/test/fixtures/es-module-loaders/babel-to-esm.js
new file mode 100644
index 000000000000..15f10f986890
--- /dev/null
+++ b/test/fixtures/es-module-loaders/babel-to-esm.js
@@ -0,0 +1,15 @@
+"use strict";
+
+/*
+created by babel with es2015 preset
+```
+export const named = 42;
+export default 84;
+```
+*/
+
+Object.defineProperty(exports, "__esModule", {
+  value: true
+});
+var named = exports.named = 42;
+exports.default = 84;
diff --git a/test/fixtures/es-module-loaders/cjs-to-es-namespace.js b/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
index 196e2d4d2f77..5df4b2b38b4a 100644
--- a/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
+++ b/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
@@ -1,6 +1,4 @@
-const { esModule } = require('module');
-
 exports.named = true;
 exports.default = 1;
 
-exports[esModule] = true;
+exports[Symbol.for('esModuleInterop')] = true;
diff --git a/test/fixtures/es-module-loaders/reserved-keywords.js b/test/fixtures/es-module-loaders/reserved-keywords.js
index 8445140a8e0f..d6935fcb7efe 100644
--- a/test/fixtures/es-module-loaders/reserved-keywords.js
+++ b/test/fixtures/es-module-loaders/reserved-keywords.js
@@ -1,8 +1,6 @@
-const { esModule } = require('module');
-
 module.exports = {
   enum: 'enum',
   class: 'class',
   delete: 'delete',
-  [esModule]: true,
+  [Symbol.for('esModuleInterop')]: true,
 };

From 4a1c95a5fc9875cd0681ec368f6995cb160a3598 Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Thu, 2 Nov 2017 14:49:01 -0500
Subject: [PATCH 07/13] symbol overrides __esModule

---
 doc/api/esm.md                                 |  8 +++++---
 lib/internal/loader/ModuleRequest.js           |  9 ++++++++-
 test/es-module/es-module.status                |  2 ++
 .../test-esm-esmoduleinterop-override.mjs      |  5 +++++
 .../es-module-loaders/babel-to-esm-override.js | 18 ++++++++++++++++++
 5 files changed, 38 insertions(+), 4 deletions(-)
 create mode 100644 test/es-module/test-esm-esmoduleinterop-override.mjs
 create mode 100644 test/fixtures/es-module-loaders/babel-to-esm-override.js

diff --git a/doc/api/esm.md b/doc/api/esm.md
index bdbaf4747b62..c1f274ef3787 100644
--- a/doc/api/esm.md
+++ b/doc/api/esm.md
@@ -85,14 +85,16 @@ or fragment string differs between `import` statements.
 
 CommonJS modules, when imported, will be handled in one of two ways. By default
 they will provide a single `default` export representing the value of
-`module.exports` at the time they finish evaluating. However, they may also
-provide `@@esModuleInterop` or `__esModule` to use named exports, representing
-each enumerable key of `module.exports` at the time they finish evaluating.
+`module.exports` at the time they finish evaluating.
+CJS modules may also provide a boolean `@@esModuleInterop` or `__esModule`
+export indicating that the enumerable keys of `module.exports` should be used
+as named exports.
 In both cases, this should be thought of  like a "snapshot" of the exports at
 the time of importing; asynchronously modifying `module.exports` will not
 affect the values of the exports. Builtin libraries are provided with named
 exports as if they were using `@@esModuleInterop`.
 
+
 ```js
 import { readFile } from 'fs';
 readFile('./foo.txt', (err, body) => {
diff --git a/lib/internal/loader/ModuleRequest.js b/lib/internal/loader/ModuleRequest.js
index 0ee9d8d3124f..c425063d014f 100644
--- a/lib/internal/loader/ModuleRequest.js
+++ b/lib/internal/loader/ModuleRequest.js
@@ -19,6 +19,9 @@ const search = require('internal/loader/search');
 const asyncReadFile = require('util').promisify(require('fs').readFile);
 const debug = require('util').debuglog('esm');
 
+const hasOwnProperty =
+  Function.prototype.call.bind(Object.prototype.hasOwnProperty);
+
 const esModuleInterop = Symbol.for('esModuleInterop');
 
 const realpathCache = new Map();
@@ -42,7 +45,8 @@ loaders.set('cjs', async (url) => {
   const CJSModule = require('module');
   const pathname = internalURLModule.getPathFromURL(new URL(url));
   const exports = CJSModule._load(pathname);
-  const es = Boolean(exports[esModuleInterop] || exports.__esModule);
+  const es = hasOwnProperty(exports, esModuleInterop) ?
+    exports[esModuleInterop] : exports.__esModule;
   const keys = es ? Object.keys(exports) : ['default'];
   return createDynamicModule(keys, url, (reflect) => {
     if (es) {
@@ -67,6 +71,9 @@ loaders.set('builtin', async (url) => {
   });
 });
 
+// Strategy for loading a native addon module
+// Named exports will not be parsed from these - see
+// https://github.com/nodejs/abi-stable-node/issues/256#issuecomment-325138872
 loaders.set('addon', async (url) => {
   const ctx = createDynamicModule(['default'], url, (reflect) => {
     debug(`Loading NativeModule ${url}`);
diff --git a/test/es-module/es-module.status b/test/es-module/es-module.status
index 971d634c2a6c..1a2794c1c87d 100644
--- a/test/es-module/es-module.status
+++ b/test/es-module/es-module.status
@@ -5,3 +5,5 @@ prefix es-module
 # sample-test                        : PASS,FLAKY
 
 [true] # This section applies to all platforms
+
+test-esm-esmoduleinterop-override       : FAIL
diff --git a/test/es-module/test-esm-esmoduleinterop-override.mjs b/test/es-module/test-esm-esmoduleinterop-override.mjs
new file mode 100644
index 000000000000..7fb297981aab
--- /dev/null
+++ b/test/es-module/test-esm-esmoduleinterop-override.mjs
@@ -0,0 +1,5 @@
+// Flags: --experimental-modules
+/* eslint-disable required-modules */
+
+import eightyfour, { named as fourtytwo }
+  from '../fixtures/es-module-loaders/babel-to-esm-override.js';
diff --git a/test/fixtures/es-module-loaders/babel-to-esm-override.js b/test/fixtures/es-module-loaders/babel-to-esm-override.js
new file mode 100644
index 000000000000..25483e5a01d5
--- /dev/null
+++ b/test/fixtures/es-module-loaders/babel-to-esm-override.js
@@ -0,0 +1,18 @@
+"use strict";
+
+/*
+created by babel with es2015 preset
+```
+export const named = 42;
+export default 84;
+```
+*/
+
+Object.defineProperty(exports, "__esModule", {
+  value: true
+});
+var named = exports.named = 42;
+exports.default = 84;
+
+// added after babel compile
+exports[Symbol.for('esModuleInterop')] = false

From 49add2b06e7e1fc6746bdc0c31185a47e05dea29 Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Thu, 2 Nov 2017 21:29:23 -0500
Subject: [PATCH 08/13] add back docs for json/c++ addons

---
 doc/api/esm.md | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/doc/api/esm.md b/doc/api/esm.md
index c1f274ef3787..77e565802ffe 100644
--- a/doc/api/esm.md
+++ b/doc/api/esm.md
@@ -83,6 +83,9 @@ All CommonJS, JSON, and C++ modules can be used with `import`.
 Modules loaded this way will only be loaded once, even if their query
 or fragment string differs between `import` statements.
 
+JSON and C++ addon modules will provide a single `default` export representing
+the value of `module.exports` at the time they finish evaluating.
+
 CommonJS modules, when imported, will be handled in one of two ways. By default
 they will provide a single `default` export representing the value of
 `module.exports` at the time they finish evaluating.

From 28f58f6781de34158df40b2641bb2df9e308c0b8 Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Fri, 3 Nov 2017 12:02:31 -0500
Subject: [PATCH 09/13] apparently this is much faster

---
 lib/internal/loader/ModuleRequest.js | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/lib/internal/loader/ModuleRequest.js b/lib/internal/loader/ModuleRequest.js
index c425063d014f..d78360f0603f 100644
--- a/lib/internal/loader/ModuleRequest.js
+++ b/lib/internal/loader/ModuleRequest.js
@@ -19,9 +19,6 @@ const search = require('internal/loader/search');
 const asyncReadFile = require('util').promisify(require('fs').readFile);
 const debug = require('util').debuglog('esm');
 
-const hasOwnProperty =
-  Function.prototype.call.bind(Object.prototype.hasOwnProperty);
-
 const esModuleInterop = Symbol.for('esModuleInterop');
 
 const realpathCache = new Map();
@@ -45,7 +42,7 @@ loaders.set('cjs', async (url) => {
   const CJSModule = require('module');
   const pathname = internalURLModule.getPathFromURL(new URL(url));
   const exports = CJSModule._load(pathname);
-  const es = hasOwnProperty(exports, esModuleInterop) ?
+  const es = exports[esModuleProp] !== undefined ?
     exports[esModuleInterop] : exports.__esModule;
   const keys = es ? Object.keys(exports) : ['default'];
   return createDynamicModule(keys, url, (reflect) => {

From c6da711c879402a7624ade428a530ec301ee04ef Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Fri, 3 Nov 2017 12:11:15 -0500
Subject: [PATCH 10/13] this is why you don't commit from github web

---
 lib/internal/loader/ModuleRequest.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/internal/loader/ModuleRequest.js b/lib/internal/loader/ModuleRequest.js
index d78360f0603f..0f2aa642db6a 100644
--- a/lib/internal/loader/ModuleRequest.js
+++ b/lib/internal/loader/ModuleRequest.js
@@ -42,7 +42,7 @@ loaders.set('cjs', async (url) => {
   const CJSModule = require('module');
   const pathname = internalURLModule.getPathFromURL(new URL(url));
   const exports = CJSModule._load(pathname);
-  const es = exports[esModuleProp] !== undefined ?
+  const es = exports[esModuleInterop] !== undefined ?
     exports[esModuleInterop] : exports.__esModule;
   const keys = es ? Object.keys(exports) : ['default'];
   return createDynamicModule(keys, url, (reflect) => {

From 1220d06c9748b40eb5083e97baad9150f151688e Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Fri, 3 Nov 2017 23:47:15 -0500
Subject: [PATCH 11/13] make tests a bit more verbose

---
 test/es-module/test-esm-cjs-esmodule.mjs                 | 2 +-
 test/es-module/test-esm-esmoduleinterop-override.mjs     | 2 +-
 test/es-module/test-esm-namespace.mjs                    | 4 ++--
 test/fixtures/es-module-loaders/babel-to-esm-override.js | 4 ++--
 test/fixtures/es-module-loaders/babel-to-esm.js          | 4 ++--
 test/fixtures/es-module-loaders/cjs-to-es-namespace.js   | 4 ++--
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/test/es-module/test-esm-cjs-esmodule.mjs b/test/es-module/test-esm-cjs-esmodule.mjs
index 1692f3505bd2..8c5cd08d65fc 100644
--- a/test/es-module/test-esm-cjs-esmodule.mjs
+++ b/test/es-module/test-esm-cjs-esmodule.mjs
@@ -2,7 +2,7 @@
 /* eslint-disable required-modules */
 
 import assert from 'assert';
-import eightyfour, { named as fourtytwo } from
+import eightyfour, { fourtytwo } from
   '../fixtures/es-module-loaders/babel-to-esm.js';
 
 assert.strictEqual(eightyfour, 84);
diff --git a/test/es-module/test-esm-esmoduleinterop-override.mjs b/test/es-module/test-esm-esmoduleinterop-override.mjs
index 7fb297981aab..47799918376e 100644
--- a/test/es-module/test-esm-esmoduleinterop-override.mjs
+++ b/test/es-module/test-esm-esmoduleinterop-override.mjs
@@ -1,5 +1,5 @@
 // Flags: --experimental-modules
 /* eslint-disable required-modules */
 
-import eightyfour, { named as fourtytwo }
+import eightyfour, { fourtytwo }
   from '../fixtures/es-module-loaders/babel-to-esm-override.js';
diff --git a/test/es-module/test-esm-namespace.mjs b/test/es-module/test-esm-namespace.mjs
index 856fdfeebb6c..335565bf7387 100644
--- a/test/es-module/test-esm-namespace.mjs
+++ b/test/es-module/test-esm-namespace.mjs
@@ -10,5 +10,5 @@ assert(fs);
 assert(fs.readFile);
 assert.strictEqual(fs.readFile, readFile);
 
-assert.strictEqual(main, 1);
-assert.strictEqual(named, true);
+assert.strictEqual(main, 'default');
+assert.strictEqual(named, 'named');
diff --git a/test/fixtures/es-module-loaders/babel-to-esm-override.js b/test/fixtures/es-module-loaders/babel-to-esm-override.js
index 25483e5a01d5..9c016023af40 100644
--- a/test/fixtures/es-module-loaders/babel-to-esm-override.js
+++ b/test/fixtures/es-module-loaders/babel-to-esm-override.js
@@ -3,7 +3,7 @@
 /*
 created by babel with es2015 preset
 ```
-export const named = 42;
+export const fourtytwo = 42;
 export default 84;
 ```
 */
@@ -11,7 +11,7 @@ export default 84;
 Object.defineProperty(exports, "__esModule", {
   value: true
 });
-var named = exports.named = 42;
+var fourtytwo = exports.fourtytwo = 42;
 exports.default = 84;
 
 // added after babel compile
diff --git a/test/fixtures/es-module-loaders/babel-to-esm.js b/test/fixtures/es-module-loaders/babel-to-esm.js
index 15f10f986890..485fa4100e01 100644
--- a/test/fixtures/es-module-loaders/babel-to-esm.js
+++ b/test/fixtures/es-module-loaders/babel-to-esm.js
@@ -3,7 +3,7 @@
 /*
 created by babel with es2015 preset
 ```
-export const named = 42;
+export const fourtytwo = 42;
 export default 84;
 ```
 */
@@ -11,5 +11,5 @@ export default 84;
 Object.defineProperty(exports, "__esModule", {
   value: true
 });
-var named = exports.named = 42;
+var fourtytwo = exports.fourtytwo = 42;
 exports.default = 84;
diff --git a/test/fixtures/es-module-loaders/cjs-to-es-namespace.js b/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
index 5df4b2b38b4a..2d767fe01b9b 100644
--- a/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
+++ b/test/fixtures/es-module-loaders/cjs-to-es-namespace.js
@@ -1,4 +1,4 @@
-exports.named = true;
-exports.default = 1;
+exports.named = 'named';
+exports.default = 'default';
 
 exports[Symbol.for('esModuleInterop')] = true;

From fe73e26878c94bfd8acfd94c9915bd79fa3b6dcc Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Sat, 4 Nov 2017 10:02:25 -0500
Subject: [PATCH 12/13] fix lint

---
 lib/internal/loader/ModuleRequest.js                 | 4 ++--
 test/es-module/test-esm-esmoduleinterop-override.mjs | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/internal/loader/ModuleRequest.js b/lib/internal/loader/ModuleRequest.js
index 0f2aa642db6a..f5a02bb7772c 100644
--- a/lib/internal/loader/ModuleRequest.js
+++ b/lib/internal/loader/ModuleRequest.js
@@ -47,7 +47,7 @@ loaders.set('cjs', async (url) => {
   const keys = es ? Object.keys(exports) : ['default'];
   return createDynamicModule(keys, url, (reflect) => {
     if (es) {
-      for (let i = 0; i < keys.length; i++)
+      for (var i = 0; i < keys.length; i++)
         reflect.exports[keys[i]].set(exports[keys[i]]);
     } else {
       reflect.exports.default.set(exports);
@@ -63,7 +63,7 @@ loaders.set('builtin', async (url) => {
   const keys = Object.keys(exports);
   return createDynamicModule(['default', ...keys], url, (reflect) => {
     reflect.exports.default.set(exports);
-    for (let i = 0; i < keys.length; i++)
+    for (var i = 0; i < keys.length; i++)
       reflect.exports[keys[i]].set(exports[keys[i]]);
   });
 });
diff --git a/test/es-module/test-esm-esmoduleinterop-override.mjs b/test/es-module/test-esm-esmoduleinterop-override.mjs
index 47799918376e..73df96c0db11 100644
--- a/test/es-module/test-esm-esmoduleinterop-override.mjs
+++ b/test/es-module/test-esm-esmoduleinterop-override.mjs
@@ -1,5 +1,6 @@
 // Flags: --experimental-modules
 /* eslint-disable required-modules */
 
+// eslint-disable-next-line no-unused-vars
 import eightyfour, { fourtytwo }
   from '../fixtures/es-module-loaders/babel-to-esm-override.js';

From c4a6c27a604961d0431eb02d76356fb414365a57 Mon Sep 17 00:00:00 2001
From: Gus Caplan <me@gus.host>
Date: Sat, 4 Nov 2017 10:16:59 -0500
Subject: [PATCH 13/13] document builtin default export

---
 doc/api/esm.md | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/api/esm.md b/doc/api/esm.md
index 77e565802ffe..fcc7108c57ae 100644
--- a/doc/api/esm.md
+++ b/doc/api/esm.md
@@ -95,7 +95,8 @@ as named exports.
 In both cases, this should be thought of  like a "snapshot" of the exports at
 the time of importing; asynchronously modifying `module.exports` will not
 affect the values of the exports. Builtin libraries are provided with named
-exports as if they were using `@@esModuleInterop`.
+exports as if they were using `@@esModuleInterop`, as well as the
+`module.exports` of the builtin provided as the `default` export.
 
 
 ```js