From 41b5cc98be1f370844b5890085517a43cf4eed66 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 27 Sep 2019 12:12:18 -0500 Subject: [PATCH 1/5] esm: remove proxy for builtin exports --- lib/internal/bootstrap/loaders.js | 136 ++++++++++++++--------- lib/internal/modules/cjs/loader.js | 8 ++ lib/internal/modules/esm/translators.js | 12 +- test/es-module/test-esm-live-binding.mjs | 24 +++- 4 files changed, 118 insertions(+), 62 deletions(-) diff --git a/lib/internal/bootstrap/loaders.js b/lib/internal/bootstrap/loaders.js index cc1157a55774e2..4759d9a10343e9 100644 --- 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,88 @@ 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; +NativeModule.prototype.syncExports = function() { + + // const update = (property, value) => { + // if (this.reflect !== undefined && + // ObjectPrototype.hasOwnProperty(this.reflect.exports, property)) + // this.reflect.exports[property].set(value); + // }; + + 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); + // 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; + // } + // }; + + // 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 index 8ef01b4499a048..61e44c3fe63b2d 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1120,6 +1120,14 @@ Module._preloadModules = function(requests) { parent.require(requests[n]); }; +Module.syncESMExports = function syncESMExports() { + 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 index 352b937766d6b5..ee1a8e0f69dcf7 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -128,14 +128,10 @@ 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}`); + const facade = module.getESMFacade(); + module.syncExports(); + return facade; }); // 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 index 5858b13bb51861..58c288c4755ce5 100644 --- 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 { syncESMExports } 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'); +syncESMExports(); assert.deepStrictEqual([fs.readFile, readFile], [undefined, undefined]); fs.readFile = fn; +syncESMExports(); assert.deepStrictEqual([fs.readFile(), readFile()], [s, s]); @@ -26,10 +29,12 @@ Reflect.defineProperty(fs, 'readFile', { configurable: true, writable: true, }); +syncESMExports(); assert.deepStrictEqual([fs.readFile(), readFile()], [s, s]); Reflect.deleteProperty(fs, 'readFile'); +syncESMExports(); assert.deepStrictEqual([fs.readFile, readFile], [undefined, undefined]); @@ -39,10 +44,14 @@ Reflect.defineProperty(fs, 'readFile', { get() { return count; }, configurable: true, }); +syncESMExports(); + +assert.deepStrictEqual([readFile, fs.readFile], [0, 0]); count++; +syncESMExports(); -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; +syncESMExports(); assert.deepStrictEqual([readFile, readFileSync], [undefined, 2]); @@ -86,17 +96,23 @@ Reflect.defineProperty(Function.prototype, 'defaultMaxListeners', { }); }, }); +syncESMExports(); assert.strictEqual(defaultMaxListeners, originDefaultMaxListeners); assert.strictEqual(events.defaultMaxListeners, originDefaultMaxListeners); -assert.strictEqual(++events.defaultMaxListeners, +events.defaultMaxListeners += 1; + +assert.strictEqual(events.defaultMaxListeners, originDefaultMaxListeners + 1); +syncESMExports(); + assert.strictEqual(defaultMaxListeners, originDefaultMaxListeners + 1); assert.strictEqual(Function.prototype.defaultMaxListeners, 1); Function.prototype.defaultMaxListeners = 'foo'; +syncESMExports(); 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 +syncESMExports(); assert.strictEqual(util.foo, 1); util.foo = 'bar'; +syncESMExports(); assert.strictEqual(count, 1); assert.strictEqual(util.foo, 'bar'); assert.strictEqual(p.foo, 2); p.foo = 'foo'; +syncESMExports(); 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'); +syncESMExports(); assert.throws( () => Object.defineProperty(events, 'defaultMaxListeners', { value: 3 }), From a961763a2bf2f90231d77701deb5a0819ee177d4 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 27 Sep 2019 12:33:41 -0500 Subject: [PATCH 2/5] reviews --- lib/internal/modules/cjs/loader.js | 2 +- lib/internal/modules/esm/translators.js | 4 +--- test/es-module/test-esm-live-binding.mjs | 30 ++++++++++++------------ 3 files changed, 17 insertions(+), 19 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 61e44c3fe63b2d..f9a9fdd9922eec 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1120,7 +1120,7 @@ Module._preloadModules = function(requests) { parent.require(requests[n]); }; -Module.syncESMExports = function syncESMExports() { +Module.syncBuiltinESMExports = function syncBuiltinESMExports() { for (const mod of NativeModule.map.values()) { if (mod.canBeRequiredByUsers) { mod.syncExports(); diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index ee1a8e0f69dcf7..f68efb88c789f5 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -129,9 +129,7 @@ translators.set('builtin', async function builtinStrategy(url) { throw new ERR_UNKNOWN_BUILTIN_MODULE(id); } debug(`Loading BuiltinModule ${url}`); - const facade = module.getESMFacade(); - module.syncExports(); - return facade; + 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 index 58c288c4755ce5..4000a621a2bd04 100644 --- a/test/es-module/test-esm-live-binding.mjs +++ b/test/es-module/test-esm-live-binding.mjs @@ -1,7 +1,7 @@ // Flags: --experimental-modules import '../common/index.mjs'; import assert from 'assert'; -import { syncESMExports } from 'module'; +import { syncBuiltinESMExports } from 'module'; import fs, { readFile, readFileSync } from 'fs'; import events, { defaultMaxListeners } from 'events'; @@ -15,12 +15,12 @@ const s = Symbol(); const fn = () => s; Reflect.deleteProperty(fs, 'readFile'); -syncESMExports(); +syncBuiltinESMExports(); assert.deepStrictEqual([fs.readFile, readFile], [undefined, undefined]); fs.readFile = fn; -syncESMExports(); +syncBuiltinESMExports(); assert.deepStrictEqual([fs.readFile(), readFile()], [s, s]); @@ -29,12 +29,12 @@ Reflect.defineProperty(fs, 'readFile', { configurable: true, writable: true, }); -syncESMExports(); +syncBuiltinESMExports(); assert.deepStrictEqual([fs.readFile(), readFile()], [s, s]); Reflect.deleteProperty(fs, 'readFile'); -syncESMExports(); +syncBuiltinESMExports(); assert.deepStrictEqual([fs.readFile, readFile], [undefined, undefined]); @@ -44,12 +44,12 @@ Reflect.defineProperty(fs, 'readFile', { get() { return count; }, configurable: true, }); -syncESMExports(); +syncBuiltinESMExports(); assert.deepStrictEqual([readFile, fs.readFile], [0, 0]); count++; -syncESMExports(); +syncBuiltinESMExports(); assert.deepStrictEqual([fs.readFile, readFile], [1, 1]); @@ -71,7 +71,7 @@ Reflect.defineProperty(fs, 'readFileSync', { }); fs.readFile = 2; -syncESMExports(); +syncBuiltinESMExports(); assert.deepStrictEqual([readFile, readFileSync], [undefined, 2]); @@ -96,7 +96,7 @@ Reflect.defineProperty(Function.prototype, 'defaultMaxListeners', { }); }, }); -syncESMExports(); +syncBuiltinESMExports(); assert.strictEqual(defaultMaxListeners, originDefaultMaxListeners); assert.strictEqual(events.defaultMaxListeners, originDefaultMaxListeners); @@ -106,13 +106,13 @@ events.defaultMaxListeners += 1; assert.strictEqual(events.defaultMaxListeners, originDefaultMaxListeners + 1); -syncESMExports(); +syncBuiltinESMExports(); assert.strictEqual(defaultMaxListeners, originDefaultMaxListeners + 1); assert.strictEqual(Function.prototype.defaultMaxListeners, 1); Function.prototype.defaultMaxListeners = 'foo'; -syncESMExports(); +syncBuiltinESMExports(); assert.strictEqual(Function.prototype.defaultMaxListeners, 'foo'); assert.strictEqual(events.defaultMaxListeners, originDefaultMaxListeners + 1); @@ -133,19 +133,19 @@ const p = { }; util.__proto__ = p; // eslint-disable-line no-proto -syncESMExports(); +syncBuiltinESMExports(); assert.strictEqual(util.foo, 1); util.foo = 'bar'; -syncESMExports(); +syncBuiltinESMExports(); assert.strictEqual(count, 1); assert.strictEqual(util.foo, 'bar'); assert.strictEqual(p.foo, 2); p.foo = 'foo'; -syncESMExports(); +syncBuiltinESMExports(); assert.strictEqual(p.foo, 'foo'); @@ -154,7 +154,7 @@ util.__proto__ = utilProto; // eslint-disable-line no-proto Reflect.deleteProperty(util, 'foo'); Reflect.deleteProperty(Function.prototype, 'defaultMaxListeners'); -syncESMExports(); +syncBuiltinESMExports(); assert.throws( () => Object.defineProperty(events, 'defaultMaxListeners', { value: 3 }), From 6c49aa3435b6564f426abda4e3307ab326402cd2 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Fri, 27 Sep 2019 13:21:00 -0500 Subject: [PATCH 3/5] docs --- doc/api/esm.md | 12 ++++--- doc/api/modules.md | 30 +++++++++++++++++ lib/internal/bootstrap/loaders.js | 54 ++----------------------------- 3 files changed, 40 insertions(+), 56 deletions(-) diff --git a/doc/api/esm.md b/doc/api/esm.md index b811f2c4cf7d3b..2ce25db24f2ac3 100644 --- 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 index 6225f41cad8365..16c20de089d434 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -984,6 +984,36 @@ const requireUtil = createRequireFromPath('../src/utils/'); requireUtil('./some-tool'); ``` +### module.syncBuiltinESMExports() + + +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 index 4759d9a10343e9..dbc21a9697e800 100644 --- a/lib/internal/bootstrap/loaders.js +++ b/lib/internal/bootstrap/loaders.js @@ -254,16 +254,9 @@ NativeModule.prototype.getESMFacade = function() { // 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. +// 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 update = (property, value) => { - // if (this.reflect !== undefined && - // ObjectPrototype.hasOwnProperty(this.reflect.exports, property)) - // this.reflect.exports[property].set(value); - // }; - const names = this.exportKeys; if (this.reflect) { for (let i = 0; i < names.length; i++) { @@ -274,49 +267,6 @@ NativeModule.prototype.syncExports = function() { ); } } - - // 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; - // } - // }; - - // this.exports = new Proxy(this.exports, handler); }; NativeModule.prototype.compile = function() { From ada2f6ee92d994296eaea62e9d423ea1815c6212 Mon Sep 17 00:00:00 2001 From: Bradley Farias Date: Tue, 1 Oct 2019 16:09:40 -0500 Subject: [PATCH 4/5] Update doc/api/modules.md Co-Authored-By: Ruben Bridgewater --- doc/api/modules.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/modules.md b/doc/api/modules.md index 16c20de089d434..0ae888bf3c7c43 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -986,7 +986,7 @@ requireUtil('./some-tool'); ### module.syncBuiltinESMExports() The `module.syncBuiltinESMExports()` method updates all the live bindings for From 089de0b27094f5859cfbba7ab7d38239f3e73241 Mon Sep 17 00:00:00 2001 From: Guy Bedford Date: Sat, 5 Oct 2019 17:25:07 -0400 Subject: [PATCH 5/5] fixup replaceme --- doc/api/modules.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/modules.md b/doc/api/modules.md index 0ae888bf3c7c43..e1ac18bcf4c0ae 100644 --- a/doc/api/modules.md +++ b/doc/api/modules.md @@ -986,7 +986,7 @@ requireUtil('./some-tool'); ### module.syncBuiltinESMExports() The `module.syncBuiltinESMExports()` method updates all the live bindings for