Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

esm: provide named exports for all builtin libraries #18131

Closed
wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jan 13, 2018

provide named exports for all builtin libraries so that the libraries may be
imported in a nicer way for esm users: import { readFile } from 'fs'
instead of importing the entire namespace, import fs from 'fs', and
calling fs.readFile

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 13, 2018
@devsnek devsnek force-pushed the esm-builtin-module-namespaces branch from f4ea88b to 90c22d0 Compare January 13, 2018 10:33
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only very lightly reviewed. No opinion on whether this is a good or bad change.

doc/api/esm.md Outdated
foo.one === 1 // true
```

Builtin modules such as will provide the above with the addition of their
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing word after 'such as.'

tools/js2c.py Outdated
var = name.replace('-', '_').replace('/', '_')
if name.endswith('.js'):
name = name.split('.', 1)[0]
var = name.replace('-', '_').replace('/', '_').replace('.', '_')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var = re.sub(r'[\-./]', '_', name)

tools/esmgen.js Outdated
@@ -0,0 +1,46 @@
const builtins = require('repl')._builtinLibs;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'use strict';

if (NativeModule.nonInternalExists(specifier)) {
if ((/^node:/.test(parentURL) &&
NativeModule.nonInternalExists(specifier.replace(/\.js$/, '')))
|| NativeModule.nonInternalExists(specifier)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operator should go on the previous line.

@devsnek devsnek force-pushed the esm-builtin-module-namespaces branch from 90c22d0 to 9000c54 Compare January 13, 2018 11:42
@guybedford
Copy link
Contributor

👍 strongly for merging this.

My only comment would be if there might not be a maintenance burden being created by this approach that might be mitigated with enumeration of the exports at load time, provided we can ensure we don't expose the wrong things (perhaps filtering _... properties etc). Would be interested to hear thoughts on this approach.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've included my comments... hope you don't mind all the questions :)

@@ -91,7 +91,7 @@ class Loader {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'url', 'string');
}

if (format === 'builtin') {
if (format === 'builtin/esm' || format === 'builtin/cjs') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need two forms of builtin? Can the builtin/esm not entirely replace the builtin/cjs? What is the use case for retaining builtin/cjs?

format: 'builtin'
};
if (/^node:/.test(parentURL) && parentURL.slice(5) === specifier) {
return { url: `${specifier}.js`, format: 'builtin/cjs' };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this case ever happen?

Copy link
Member Author

@devsnek devsnek Jan 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the esm wrapper imports the actual cjs it looks like node:assert requesting assert, and thats the only time the builtin cjs should get used

tools/esmgen.js Outdated
test.push(`check(${name}, ${key});`);
}

console.log(test.join('\n'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the assumption that this file is run periodically to update the lib folder? Is there a way to make this more automated? Why would you think this level of automation is preferable to runtime enumeration?

Copy link
Member Author

@devsnek devsnek Jan 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hopefully it never needs to be used again, but i still included it since it might be useful at some point, maybe during a refactor or something in the far future

@devsnek
Copy link
Member Author

devsnek commented Jan 13, 2018

@guybedford i chose this over runtime enumeration because of the results in my last pr where i attempted forward evaluation but i guess if we can guarantee our libs won't have errors on evaluation it should be fine? i don't think our public api changes that much for it to be a real burden. this also lays groundwork for writing core parts of node in esm, although i don't know if that matters much to people. it should be noted that if we switch to runtime enumerating with out of order evaluation then the current issues i'm having with tests will also go away, and it will get rid of people who make loaders worrying about resolving both kinds builtins, which is definitely a confusing process.

@guybedford
Copy link
Contributor

@devsnek thanks for clarifying. Personally I think the runtime creation would be better as it would avoid the need for the new builtin interpretation mode, remove the maintenance burden of maintaining the wrappers (generated or not), and it would effectively be exactly the same algorithm to build up the exports, saving an extra file load anyway to do that.

@devsnek
Copy link
Member Author

devsnek commented Jan 13, 2018

@guybedford i just know there was significant opposition to out-of-order evaluation in my previous pr, but maybe @bmeck its fine in this case?

@guybedford
Copy link
Contributor

guybedford commented Jan 13, 2018

Personally I don't seen an issue with considering core modules "preevaluated" (from the esm loader perspective). It's user dependencies with circular references and errors that we have to worry about for that issue.

@bmeck
Copy link
Member

bmeck commented Jan 13, 2018

I don't like that the exports can go out of sync but am not blocking that at this point. I'd love it if the backing object could stay in sync for a variety of reasons, but all approaches I've tested are problematic for performance.

@bmeck
Copy link
Member

bmeck commented Jan 13, 2018

@devsnek could you make the ESM facades eagerly populate even when required so that they stay the true primordial form of the exports?

@devsnek
Copy link
Member Author

devsnek commented Jan 13, 2018

@bmeck i'm not sure what you mean by "true primordial form" but my method would basically be:

loaders.set('builtin', async (url) => {
  const module = InternalModule.require(url.slice(5));
  const properties = Object.getOwnPropertyDescriptors(module);
  const keys = ['default'];
  for (const [name, prop] of Object.entries(properties)) {
    if (!prop.enumerable || !prop.value)
      continue;
    if (/(^_)|(_$)/.test(name))
      continue;
    keys.push(name);
  }
  return createDynamicModule(keys, url, (reflect) => {
    reflect.exports.default.set(module);
    for (const key of keys)
      reflect.exports[key].set(module[key]);
  });
});

@bmeck
Copy link
Member

bmeck commented Jan 13, 2018

@devsnek I just want to be sure that if you mutate fs it does not mutate the named export value depending on timing.

const fs = require('fs');
fs.readFile = () => {}
import {readFile} from 'fs';
// should never be that noop function (even if this file is loaded after the one above)

@devsnek
Copy link
Member Author

devsnek commented Jan 13, 2018

@bmeck i don't know of any way to guarantee at all, even requiring every builtin before user code runs and keeping a cache would still allow user mutation of the exports object.

@guybedford
Copy link
Contributor

@devsnek the same sort of technique we use to eagerly inject CJS modules into the loader registry for this should work here for the core modules I think?

@guybedford
Copy link
Contributor

(I know it's not pretty, but it provides the invariants)

lib/assert.mjs Outdated
@@ -0,0 +1,19 @@
/* eslint-disable no-restricted-properties */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought... could we not devise a mechanism for generating these automatically during the build so we do not need to keep them manually in sync? The surface area of core modules is fixed at time of build.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasnell we could somehow with static parsing maybe, but various things like getter/setters would need to be excluded.

lib/assert.mjs Outdated
export const notStrictEqual = assert.notStrictEqual;
export const throws = assert.throws;
export const doesNotThrow = assert.doesNotThrow;
export const ifError = assert.ifError;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears to be missing the new strict export.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it might not have been in my branch when i generated these, but since i'm changing this to generate the exports at runtime it shouldn't be an issue. (stay tuned!! 😄)

lib/crypto.mjs Outdated
export const pbkdf2Sync = crypto.pbkdf2Sync;
export const privateDecrypt = crypto.privateDecrypt;
export const privateEncrypt = crypto.privateEncrypt;
export const prng = crypto.prng;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should decide if we really want to export pure aliases or take the opportunity to begin limiting access to those

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you expand on the reason to limit access to things?

lib/domain.mjs Outdated
@@ -0,0 +1,7 @@
import domain from 'domain';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we export deprecated modules at all?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+0 to removing them, not blocking on it

@devsnek devsnek force-pushed the esm-builtin-module-namespaces branch from 9000c54 to 43bfe9e Compare January 13, 2018 18:03
@devsnek devsnek changed the title esm: provide wrappers for all builtin libraries esm: provide named exports for all builtin libraries Jan 13, 2018
@devsnek devsnek force-pushed the esm-builtin-module-namespaces branch from 43bfe9e to e46ccb8 Compare January 13, 2018 18:05
@guybedford
Copy link
Contributor

New approach looks good.

Copy link
Member

@bmeck bmeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

const descriptors =
Object.getOwnPropertyDescriptors(nativeModule.exports);
for (const [name, d] of Object.entries(descriptors)) {
if (d.setter !== undefined || d.getter !== undefined || !d.enumerable)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is "setter" and "getter"? Property descriptors have set and get.

jasnell
jasnell previously approved these changes Jan 13, 2018
@devsnek devsnek force-pushed the esm-builtin-module-namespaces branch from e46ccb8 to c63e761 Compare January 13, 2018 20:43
@devsnek devsnek force-pushed the esm-builtin-module-namespaces branch from f2627aa to e1770a2 Compare April 23, 2018 06:31
Reflect.defineProperty(target, prop, descriptor)) {
update(prop, valueDescriptor.value);
return true;
}
Copy link
Member

@jdalton jdalton Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👆 In the if (valueDescriptor && condition you can make it if (valueDescriptor) { then inside the block capture the result of Reflect.defineProperty(target, prop, descriptor), call update and return the captured result.

if (this.namespace.includes(prop))
return false;
return delete target[prop];
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👆 By returning false you're making the CJS export objects non-delete able. Instead you could allow the operation to happen, capturing the result of Reflect.deleteProperty(target, prop), calling update, then returning the captured result. If a property is deleted its updated value is undefined or whatever is exposed on its prototype.

Copy link
Member Author

@devsnek devsnek Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calling update

calling it with what exactly? the property is gone if we let it get deleted

Copy link
Member

@jdalton jdalton Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calling it with what exactly? the property is gone if we let it get deleted

Whatever the value of target[prop] that's remaining.

@devsnek devsnek force-pushed the esm-builtin-module-namespaces branch from a3284c6 to 2991f5c Compare April 23, 2018 07:12
}
return value;
},
});
Copy link
Member

@jdalton jdalton Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take a slightly different approach to wrapping. Instead of creating a new wrap function, I proxy the original.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why you would prefer that?

Copy link
Member

@jdalton jdalton Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu Yes. I first avoid wrapping for the 99% case (plain or bound functions) and then only wrap with a proxy for the native case. For the 1% case (native method) wrapping with a proxy lets all other traps passthru to the original function while I just hook the apply trap.

Beyond the fact that less wrapping is good in general this is important to the esm loader because folks can opt for CJS named export support beyond buitlins. Avoiding the function/proxy wrap means more methods are === to other references a user might store.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to re-audit our exports to make sure we only need to bind for native methods, but that does seem like a better case

Copy link
Member Author

@devsnek devsnek Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton your proxy unfortunately breaks in certain cases which i think could be a v8 bug so i'll keep using my wrap function for now

@nodejs/v8

> Reflect.apply(EventEmitter.call, EventEmitter, [])
TypeError: Reflect.apply is not a function

Copy link
Member Author

@devsnek devsnek Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton i wasn't running any esm, it broke regular usage of event emitter. it seems like v8 doesn't enjoy Function.prototype.call and Reflect.apply together, this isn't just an issue with EventEmitter.

var EventEmitter = require('events');

// probably some ghetto es3 function extending EventEmitter
EventEmitter.call(...); 

i can special case the thisArg variable like value === Function.prototype.call but that feels nasty and the wrap works

Copy link
Member

@jdalton jdalton Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dev error? Proxies should work.
Can you show how you attempted proxies. Might be able to spot the issue.

In my implementation

EventEmitter.call({})

works. The .call is proxy wrapped.

I have smth like this for the handler (poke around the implementation here)

wrapper = new Proxy(value, {
  apply(funcTarget, thisArg, args) {
    if (thisArg === proxy ||
        thisArg === entry.esmNamespace) {
      thisArg = target
    }

    return Reflect.apply(value, thisArg, args)
  }
})

When EventEmitter.call({}) is called the thisArg === proxy condition is met because var EventEmitter is the events module module.exports proxy. The thisArg is set to the original target (the unwrapped module.exports of events). This results in a successful invocation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devsnek Can you find an isolated reproduction of the bug? The following seems to be working just fine here:

$ node
> const { EventEmitter } = events
undefined
> Reflect.apply(EventEmitter.call, EventEmitter, [])
TypeError: Cannot set property 'domain' of undefined
    at EventEmitter.init (domain.js:401:15)
    at EventEmitter (events.js:27:21)

Copy link
Member Author

@devsnek devsnek Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdalton i copied yours in, except for using regular proxies instead of your OwnProxy, which i doubt makes any difference in this case

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok i just came back to this and it seems like the proxy works now so i'm going to just assume i need more sleep... i'll push in a few minutes after some more testing

lib/util.js Outdated
@@ -433,7 +436,7 @@ function formatValue(ctx, value, recurseTimes, ln) {
return ctx.stylize('null', 'null');
}

if (ctx.showProxy) {
if (ctx.showProxy && !nativeModuleProxies.has(value)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should make a distinction between "our proxies" versus "their proxies".

We should either disable proxy showing by default, or just show the proxy.

Copy link
Member

@jdalton jdalton Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TimothyGu It's cosmetic and can totally be tackled in a follow-up PR. The idea from earlier in the thread was that folks thought it would be less-good displaying the Proxy prefix when inspecting builtin module exports in the repl.

Copy link
Member

@TimothyGu TimothyGu Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to argue in the other way: because it is cosmetic, this change can be done in a follow-up PR. Changes should be atomic and focused on one topic at a time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to argue in the other way: because it is cosmetic, this change can be done in a follow-up PR.

We're on the same page. I was saying that the masking of builtin exports could be done in a follow-up PR instead of this one 😁

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops 😛

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress, but a bit more work is still needed.


Also, how much does this slow down require()ing an internal module (and Node.js startup) from CJS with the --experimental-modules flag turned on? We could get this in with a performance hit, but we need to be able to quantify that and know what to fix in the future.

},
defineProperty(target, prop, descriptor) {
if (Reflect.defineProperty(target, prop, descriptor)) {
update(prop, Reflect.get(target, prop, target));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last , target is unneeded.

const exports = NativeModule.require(url.slice(5));
reflect.exports.default.set(exports);
});
const id = url.substr(5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What exactly does this do? A comment would help with what exactly this truncates. Also we generally use .slice() (substr is part of Annex B).

@@ -1,10 +1,11 @@
import _url from 'url';
import { URL } from 'url';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to import URL at all in Node.js v10.x :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you never know where this might be backported to

this.namespace = Object.entries(
Object.getOwnPropertyDescriptors(this.exports))
.filter(([name, d]) => d.enumerable)
.map(([name]) => name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Core code rarely use the functional array methods. Let's make this imperative instead.

this.namespace = [];
for (const key of Object.getOwnPropertyNames(this.exports)) {
  const desc = Object.getOwnPropertyDescriptor(this.exports, key);
  if (!desc.enumerable)
    continue;
  namespace.push(key);
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mfw thats what i was using before 😢 will change back

const proxy = new Proxy(this.exports, {
set(target, prop, value) {
if (Reflect.set(target, prop, value)) {
update(prop, value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with defineProperty, use update(prop, Reflect.get(target, prop)). (This behavior is observable through getters/setters.)

const methodWrapMap = new WeakMap();

const proxy = new Proxy(this.exports, {
set(target, prop, value) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set hook has a fourth receiver argument that you are not handling.
You need to at least make sure to forward the receiver argument to Reflect.set.

}
return false;
},
get(target, prop) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to handle receiver here as well.

wrap.prototype = value.prototype;
methodWrapMap.set(value, wrap);
return wrap;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might need to add this handling for getOwnPropertyDescriptor as well.

fs.readFile = () => s;

assert.strictEqual(fs.readFile(), s);
assert.strictEqual(readFile(), s);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still need to test:

  • delete
  • delete and set afterwards
  • set
  • defineProperty
  • All of the above, but with accessor properties

@devsnek devsnek force-pushed the esm-builtin-module-namespaces branch 2 times, most recently from 716aec6 to c40ac4d Compare April 24, 2018 02:04
for (const key of Object.getOwnPropertyNames(this.exports)) {
const desc = Object.getOwnPropertyDescriptor(this.exports, key);
if (desc.enumerable)
this.namespace.push(key);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of getOwnPropertyNames+enumerable check you could use Object.keys

@devsnek
Copy link
Member Author

devsnek commented Apr 24, 2018

@TimothyGu

average startup time with child process + loop + console.log(perf_hooks.performance.nodeTiming) + shameless rounding:

without proxy (1000 runs) 79.71845515598729
with proxy (1000 runs) 90.33686945802346

i didn't bother testing time of an individual require because it can only happen at max once. its also worth nothing this increased time should be more than handled by snapshots, whenever we finish those.


if (typeof value.name === 'string' && /^bound /.test(value.name))
return value;

Copy link
Member

@jdalton jdalton Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides non-functions and bound-functions you can also skip non-native functions.
I have an inference method here for reference. (a v8 helper for this would be rad++)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't skip native functions. for instance if you do module.exports = new Map() and we don't wrap the exports then Map.prototype.* will have improper receivers and throw

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can't skip native functions.

I know, I'm saying skip non-native.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the only functions we bind then are unbound native functions? what about member functions written in js

Copy link
Member

@jdalton jdalton Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the only functions we bind then are unbound native functions? what about member functions written in js

We aren't binding functions. The wrapper juggles the thisArg around for the one, maybe two, cases that cause native methods grief but beyond that we forward the thisArg along. So it makes sense to only wrap the methods that need the thisArg juggling in the first place (native methods).

apply(t, thisArg, args) {
if (thisArg === proxy)
thisArg = target;
return Reflect.apply(t, thisArg, args);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might end up needing a thisArg === nsObj check.
A test for calling a native method on the namespace object would cover it.

@devsnek devsnek force-pushed the esm-builtin-module-namespaces branch 2 times, most recently from 0975efa to cb20994 Compare April 28, 2018 21:18
@devsnek
Copy link
Member Author

devsnek commented Apr 28, 2018

alright so this is the 400th comment in this thread and i'm starting to consistently get the github unicorn when loading this. at the moment it works and this pr seems to be ready. that being said, i need some approvals on this. i would love to land this by the end of next week.

@jdalton
Copy link
Member

jdalton commented Apr 28, 2018

👍 As an initial landing of an experimental feature I think it's great. Every time I look at this implementation I find something in my own to improve! There are some things to dry-up and tweak here but they can be tackled in follow-up PRs.

provide named exports for all builtin libraries so that the libraries may be
imported in a nicer way for esm users: `import { readFile } from 'fs'`
instead of importing the entire namespace, `import fs from 'fs'`, and
calling `fs.readFile`. the default export is left as the entire
namespace (module.exports)
@devsnek devsnek force-pushed the esm-builtin-module-namespaces branch from cb20994 to 3aabe35 Compare April 29, 2018 03:03
@giltayar
Copy link
Contributor

giltayar commented Apr 29, 2018 via email

this.exportKeys = Object.keys(this.exports);

const update = (property, value) => {
if (this.reflect !== undefined && this.exportKeys.includes(property))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it ok that delete Array.prototype.includes can break this code?

If not, you could copy Array.prototype.includes to be an own property on this.exportKeys, perhaps?

(same question on has/get/set on collection instances)

Copy link
Member

@jdalton jdalton Apr 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ljharb There's another issue (here) for isolating internals to avoid ad hoc primordial scaffolding in each module. Trying to isolate includes, Object.keys, Reflect or other is probably out of scope for this specific PR (esp. since it's experimental and the issue is larger than this PR).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, just wanted to call it out :-)

NativeModule.require(id);
const module = NativeModule.getCached(id);
return createDynamicModule(
[...module.exportKeys, 'default'], url, (reflect) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm not sure if the ordering matters here at all - is default always last?

https://tc39.github.io/ecma262/#sec-modulenamespacecreate step 7 suggests that all export keys, including "default" if present, should be alphabetically sorted. (i do see at least one test that validates the ordering, but i'm not sure if that test covers this code or not)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the order doesn't matter there actually as it just gets injected into a generated source text

@guybedford
Copy link
Contributor

My greatest concern here is the potential 10% performance slowdown for NodeJS app startup having all core modules as proxies in CommonJS code, as Gus provided in some numbers at #18131 (comment).

It may turn out that creating setter-based core modules could be an alternative to the proxy approach that is also faster, so I do think this would still be worth seriously considering, or at least comparing for performance.

The benefit of a proxy over just a setter is supporting dynamic properties and object.defineProperty configuration cases. But dynamic properties are already not supported, so perhaps benefits may be worth the loss of configuration hooks.

@devsnek
Copy link
Member Author

devsnek commented Apr 29, 2018

@guybedford as long as the experimental flag is around i'd rather take it as an opportunity to experiment with the behaviour rather than perf optimisation. come time to ship it we can always make it more performant if needed. i'm also exceedingly hopeful that we will finish up snapshots by that time and then we won't have to worry this at all

@guybedford
Copy link
Contributor

guybedford commented Apr 29, 2018 via email

@devsnek
Copy link
Member Author

devsnek commented Apr 29, 2018

@guybedford this code doesn't run unless the flag is given, and yes i would agree a new pr is probably a good idea

@devsnek devsnek closed this Apr 29, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
Docs-only deprecate the getter/setter crypto.fips and replace
with crypto.setFips() and crypto.getFips()

This is specifically in preparation for ESM module support

PR-URL: nodejs#18335
Refs: nodejs#18131
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Runtime deprecate the crypto.DEFAULT_ENCODING property.

This is specifically in preparation for eventual ESM support
Refs: nodejs#18131

PR-URL: nodejs#18333
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
Docs-only deprecate the getter/setter crypto.fips and replace
with crypto.setFips() and crypto.getFips()

This is specifically in preparation for ESM module support

PR-URL: nodejs#18335
Refs: nodejs#18131
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.