Skip to content

Commit

Permalink
esm: remove proxy for builtin exports
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
bfarias-godaddy authored and guybedford committed Oct 6, 2019
1 parent e1e2f66 commit 0b495a8
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 69 deletions.
12 changes: 8 additions & 4 deletions doc/api/esm.md
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;
```
Expand Down Expand Up @@ -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
Expand Down
30 changes: 30 additions & 0 deletions doc/api/modules.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
92 changes: 37 additions & 55 deletions lib/internal/bootstrap/loaders.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
};
Expand All @@ -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() {
Expand Down
8 changes: 8 additions & 0 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
10 changes: 2 additions & 8 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 22 additions & 2 deletions test/es-module/test-esm-live-binding.mjs
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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]);

Expand All @@ -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]);

Expand All @@ -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;

Expand All @@ -62,6 +71,7 @@ Reflect.defineProperty(fs, 'readFileSync', {
});

fs.readFile = 2;
syncBuiltinESMExports();

assert.deepStrictEqual([readFile, readFileSync], [undefined, 2]);

Expand All @@ -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);
Expand All @@ -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');

Expand All @@ -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 }),
Expand Down

0 comments on commit 0b495a8

Please sign in to comment.