diff options
author | Bradley Farias <bfarias@godaddy.com> | 2019-09-27 12:12:18 -0500 |
---|---|---|
committer | Guy Bedford <guybedford@gmail.com> | 2019-10-05 20:11:15 -0400 |
commit | 0b495a899bdf9a26b25a6e31177512531c841e0e (patch) | |
tree | d0c3a6b3768c0882ddf2eae1a01df419b50bb38f | |
parent | e1e2f669f65fd53323b8a58d80ed3cee039706b7 (diff) | |
download | android-node-v8-0b495a899bdf9a26b25a6e31177512531c841e0e.tar.gz android-node-v8-0b495a899bdf9a26b25a6e31177512531c841e0e.tar.bz2 android-node-v8-0b495a899bdf9a26b25a6e31177512531c841e0e.zip |
esm: remove proxy for builtin exports
PR-URL: https://github.com/nodejs/node/pull/29737
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
-rw-r--r-- | doc/api/esm.md | 12 | ||||
-rw-r--r-- | doc/api/modules.md | 30 | ||||
-rw-r--r-- | lib/internal/bootstrap/loaders.js | 92 | ||||
-rw-r--r-- | lib/internal/modules/cjs/loader.js | 8 | ||||
-rw-r--r-- | lib/internal/modules/esm/translators.js | 10 | ||||
-rw-r--r-- | test/es-module/test-esm-live-binding.mjs | 24 |
6 files changed, 107 insertions, 69 deletions
diff --git a/doc/api/esm.md b/doc/api/esm.md index 5805284016..e0a548f042 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 32490cf47f..9ed9273e0b 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() +<!-- 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 index cc1157a557..dbc21a9697 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,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 index 9bee9130d2..862b149e5a 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1135,6 +1135,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 index 056bf64bf5..e8eddcfd21 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -128,14 +128,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 index 5858b13bb5..4000a621a2 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 { 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 }), |