esm: remove proxy for builtin exports

PR-URL: #29737 Reviewed-By: Guy Bedford <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Jan Krems <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
nodejs · Oct 9, 2019 · e7f604f · e7f604f
1 parent ae8b2b4
commit e7f604f
Show file tree

Hide file tree

Showing 6 changed files with 107 additions and 69 deletions.
diff --git a/doc/api/esm.md b/doc/api/esm.md
@@ -555,10 +555,11 @@ cjs === 'cjs'; // true
 
 ## Builtin modules
 
-Builtin modules will provide named exports of their public API, as well as a
-default export which can be used for, among other things, modifying the named
-exports. Named exports of builtin modules are updated when the corresponding
-exports property is accessed, redefined, or deleted.
+Builtin modules will provide named exports of their public API. A
+default export is also provided which is the value of the CommonJS exports.
+The default export can be used for, among other things, modifying the named
+exports. Named exports of builtin modules are updated only by calling
+[`module.syncBuiltinESMExports()`][].
 
 ```js
 import EventEmitter from 'events';
@@ -578,8 +579,10 @@ readFile('./foo.txt', (err, source) => {
 
 ```js
 import fs, { readFileSync } from 'fs';
+import { syncBuiltinESMExports } from 'module';
 
 fs.readFileSync = () => Buffer.from('Hello, ESM');
+syncBuiltinESMExports();
 
 fs.readFileSync === readFileSync;
 ```
@@ -1008,6 +1011,7 @@ success!
 [`import.meta.url`]: #esm_import_meta
 [`import`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import
 [`module.createRequire()`]: modules.html#modules_module_createrequire_filename
+[`module.syncBuiltinESMExports()`]: modules.html#modules_module_syncbuiltinesmexports
 [dynamic instantiate hook]: #esm_dynamic_instantiate_hook
 [package exports]: #esm_package_exports
 [special scheme]: https://url.spec.whatwg.org/#special-scheme

diff --git a/doc/api/modules.md b/doc/api/modules.md
@@ -984,6 +984,36 @@ const requireUtil = createRequireFromPath('../src/utils/');
 requireUtil('./some-tool');
 ```
 
+### module.syncBuiltinESMExports()
+<!-- YAML
+added: REPLACEME
+-->
+
+The `module.syncBuiltinESMExports()` method updates all the live bindings for
+builtin ES Modules to match the properties of the CommonJS exports. It does
+not add or remove exported names from the ES Modules.
+
+```js
+const fs = require('fs');
+const { syncBuiltinESMExports } = require('module');
+
+fs.readFile = null;
+
+delete fs.readFileSync;
+
+fs.newAPI = function newAPI() {
+  // ...
+};
+
+syncBuiltinESMExports();
+
+import('fs').then((esmFS) => {
+  assert.strictEqual(esmFS.readFile, null);
+  assert.strictEqual('readFileSync' in fs, true);
+  assert.strictEqual(esmFS.newAPI, undefined);
+});
+```
+
 [GLOBAL_FOLDERS]: #modules_loading_from_the_global_folders
 [`Error`]: errors.html#errors_class_error
 [`__dirname`]: #modules_dirname

diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js
@@ -151,6 +151,7 @@ function NativeModule(id) {
   this.id = id;
   this.exports = {};
   this.reflect = undefined;
+  this.esmFacade = undefined;
   this.exportKeys = undefined;
   this.loaded = false;
   this.loading = false;
@@ -211,15 +212,19 @@ function requireWithFallbackInDeps(request) {
 }
 
 // This is exposed for public loaders
-NativeModule.prototype.compileForPublicLoader = function(needToProxify) {
+NativeModule.prototype.compileForPublicLoader = function(needToSyncExports) {
   if (!this.canBeRequiredByUsers) {
     // No code because this is an assertion against bugs
     // eslint-disable-next-line no-restricted-syntax
     throw new Error(`Should not compile ${this.id} for public use`);
   }
   this.compile();
-  if (needToProxify && !this.exportKeys) {
-    this.proxifyExports();
+  if (needToSyncExports) {
+    if (!this.exportKeys) {
+      this.exportKeys = Object.keys(this.exports);
+    }
+    this.getESMFacade();
+    this.syncExports();
   }
   return this.exports;
 };
@@ -230,61 +235,38 @@ const getOwn = (target, property, receiver) => {
     undefined;
 };
 
+NativeModule.prototype.getURL = function() {
+  return `node:${this.id}`;
+};
+
+NativeModule.prototype.getESMFacade = function() {
+  if (this.esmFacade) return this.esmFacade;
+  const createDynamicModule = nativeModuleRequire(
+    'internal/modules/esm/create_dynamic_module');
+  const url = this.getURL();
+  return this.esmFacade = createDynamicModule(
+    [], [...this.exportKeys, 'default'], url, (reflect) => {
+      this.reflect = reflect;
+      this.syncExports();
+      reflect.exports.default.set(this.exports);
+    });
+};
+
 // Provide named exports for all builtin libraries so that the libraries
 // may be imported in a nicer way for ESM users. The default export is left
-// as the entire namespace (module.exports) and wrapped in a proxy such
-// that APMs and other behavior are still left intact.
-NativeModule.prototype.proxifyExports = function() {
-  this.exportKeys = Object.keys(this.exports);
-
-  const update = (property, value) => {
-    if (this.reflect !== undefined &&
-        ObjectPrototype.hasOwnProperty(this.reflect.exports, property))
-      this.reflect.exports[property].set(value);
-  };
-
-  const handler = {
-    __proto__: null,
-    defineProperty: (target, prop, descriptor) => {
-      // Use `Object.defineProperty` instead of `Reflect.defineProperty`
-      // to throw the appropriate error if something goes wrong.
-      Object.defineProperty(target, prop, descriptor);
-      if (typeof descriptor.get === 'function' &&
-          !Reflect.has(handler, 'get')) {
-        handler.get = (target, prop, receiver) => {
-          const value = Reflect.get(target, prop, receiver);
-          if (ObjectPrototype.hasOwnProperty(target, prop))
-            update(prop, value);
-          return value;
-        };
-      }
-      update(prop, getOwn(target, prop));
-      return true;
-    },
-    deleteProperty: (target, prop) => {
-      if (Reflect.deleteProperty(target, prop)) {
-        update(prop, undefined);
-        return true;
-      }
-      return false;
-    },
-    set: (target, prop, value, receiver) => {
-      const descriptor = Reflect.getOwnPropertyDescriptor(target, prop);
-      if (Reflect.set(target, prop, value, receiver)) {
-        if (descriptor && typeof descriptor.set === 'function') {
-          for (const key of this.exportKeys) {
-            update(key, getOwn(target, key, receiver));
-          }
-        } else {
-          update(prop, getOwn(target, prop, receiver));
-        }
-        return true;
-      }
-      return false;
+// as the entire namespace (module.exports) and updates when this function is
+// called so that APMs and other behavior are supported.
+NativeModule.prototype.syncExports = function() {
+  const names = this.exportKeys;
+  if (this.reflect) {
+    for (let i = 0; i < names.length; i++) {
+      const exportName = names[i];
+      if (exportName === 'default') continue;
+      this.reflect.exports[exportName].set(
+        getOwn(this.exports, exportName, this.exports)
+      );
     }
-  };
-
-  this.exports = new Proxy(this.exports, handler);
+  }
 };
 
 NativeModule.prototype.compile = function() {

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
@@ -1130,6 +1130,14 @@ Module._preloadModules = function(requests) {
     parent.require(requests[n]);
 };
 
+Module.syncBuiltinESMExports = function syncBuiltinESMExports() {
+  for (const mod of NativeModule.map.values()) {
+    if (mod.canBeRequiredByUsers) {
+      mod.syncExports();
+    }
+  }
+};
+
 // Backwards compatibility
 Module.Module = Module;
 

diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
@@ -129,14 +129,8 @@ translators.set('builtin', async function builtinStrategy(url) {
   if (!module) {
     throw new ERR_UNKNOWN_BUILTIN_MODULE(id);
   }
-  return createDynamicModule(
-    [], [...module.exportKeys, 'default'], url, (reflect) => {
-      debug(`Loading BuiltinModule ${url}`);
-      module.reflect = reflect;
-      for (const key of module.exportKeys)
-        reflect.exports[key].set(module.exports[key]);
-      reflect.exports.default.set(module.exports);
-    });
+  debug(`Loading BuiltinModule ${url}`);
+  return module.getESMFacade();
 });
 
 // Strategy for loading a JSON file

diff --git a/test/es-module/test-esm-live-binding.mjs b/test/es-module/test-esm-live-binding.mjs
@@ -1,6 +1,7 @@
 // Flags: --experimental-modules
 import '../common/index.mjs';
 import assert from 'assert';
+import { syncBuiltinESMExports } from 'module';
 
 import fs, { readFile, readFileSync } from 'fs';
 import events, { defaultMaxListeners } from 'events';
@@ -14,10 +15,12 @@ const s = Symbol();
 const fn = () => s;
 
 Reflect.deleteProperty(fs, 'readFile');
+syncBuiltinESMExports();
 
 assert.deepStrictEqual([fs.readFile, readFile], [undefined, undefined]);
 
 fs.readFile = fn;
+syncBuiltinESMExports();
 
 assert.deepStrictEqual([fs.readFile(), readFile()], [s, s]);
 
@@ -26,10 +29,12 @@ Reflect.defineProperty(fs, 'readFile', {
   configurable: true,
   writable: true,
 });
+syncBuiltinESMExports();
 
 assert.deepStrictEqual([fs.readFile(), readFile()], [s, s]);
 
 Reflect.deleteProperty(fs, 'readFile');
+syncBuiltinESMExports();
 
 assert.deepStrictEqual([fs.readFile, readFile], [undefined, undefined]);
 
@@ -39,10 +44,14 @@ Reflect.defineProperty(fs, 'readFile', {
   get() { return count; },
   configurable: true,
 });
+syncBuiltinESMExports();
+
+assert.deepStrictEqual([readFile, fs.readFile], [0, 0]);
 
 count++;
+syncBuiltinESMExports();
 
-assert.deepStrictEqual([readFile, fs.readFile, readFile], [0, 1, 1]);
+assert.deepStrictEqual([fs.readFile, readFile], [1, 1]);
 
 let otherValue;
 
@@ -62,6 +71,7 @@ Reflect.defineProperty(fs, 'readFileSync', {
 });
 
 fs.readFile = 2;
+syncBuiltinESMExports();
 
 assert.deepStrictEqual([readFile, readFileSync], [undefined, 2]);
 
@@ -86,17 +96,23 @@ Reflect.defineProperty(Function.prototype, 'defaultMaxListeners', {
     });
   },
 });
+syncBuiltinESMExports();
 
 assert.strictEqual(defaultMaxListeners, originDefaultMaxListeners);
 assert.strictEqual(events.defaultMaxListeners, originDefaultMaxListeners);
 
-assert.strictEqual(++events.defaultMaxListeners,
+events.defaultMaxListeners += 1;
+
+assert.strictEqual(events.defaultMaxListeners,
                    originDefaultMaxListeners + 1);
 
+syncBuiltinESMExports();
+
 assert.strictEqual(defaultMaxListeners, originDefaultMaxListeners + 1);
 assert.strictEqual(Function.prototype.defaultMaxListeners, 1);
 
 Function.prototype.defaultMaxListeners = 'foo';
+syncBuiltinESMExports();
 
 assert.strictEqual(Function.prototype.defaultMaxListeners, 'foo');
 assert.strictEqual(events.defaultMaxListeners, originDefaultMaxListeners + 1);
@@ -117,16 +133,19 @@ const p = {
 };
 
 util.__proto__ = p; // eslint-disable-line no-proto
+syncBuiltinESMExports();
 
 assert.strictEqual(util.foo, 1);
 
 util.foo = 'bar';
+syncBuiltinESMExports();
 
 assert.strictEqual(count, 1);
 assert.strictEqual(util.foo, 'bar');
 assert.strictEqual(p.foo, 2);
 
 p.foo = 'foo';
+syncBuiltinESMExports();
 
 assert.strictEqual(p.foo, 'foo');
 
@@ -135,6 +154,7 @@ util.__proto__ = utilProto; // eslint-disable-line no-proto
 
 Reflect.deleteProperty(util, 'foo');
 Reflect.deleteProperty(Function.prototype, 'defaultMaxListeners');
+syncBuiltinESMExports();
 
 assert.throws(
   () => Object.defineProperty(events, 'defaultMaxListeners', { value: 3 }),