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] 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 d8143da378f768..f3bb4956941435 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 72f3dd3ee570c2..50ce90bb73ea91 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 72b7fed4b33dfa..d29d6d3ee8af7d 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);