Missing word after 'such as.'

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

'use strict';

Operator should go on the previous line.

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?

When does this case ever happen?

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

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?

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

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.

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

This appears to be missing the new strict export.

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!! 😄)

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

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

Should we export deprecated modules at all?

+0 to removing them, not blocking on it

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

I’d think it’d be sufficient to show that fs.readFileSync === readFileSync?

Should this use hasOwnProperty instead of in?

This won’t be robust against Function.toString.call, fwiw

that's just left over from some other stuff i was doing, i'll remove it

Similarly, this doesn’t have to actually have a function - it just needs something to ===

because this implementation wraps functions, i feel like testing return value is worthwhile

👆 Why is events special cased?

i don't remember anymore but at some point everyone was fine with this :/

Can we side on removing unnecessary filtering please.

👆 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.

👆 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.

calling update

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

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

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

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

Is there a reason why you would prefer that?

@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.

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

@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

@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

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.

@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)

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

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

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.

@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.

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.

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 😁

Oops 😛

Last , target is unneeded.

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).

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

you never know where this might be backported to

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);
}

mfw thats what i was using before 😢 will change back

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

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.

You need to handle receiver here as well.

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

Still need to test:

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

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

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++)

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

we can't skip native functions.

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

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

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).

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

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)

@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).

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

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)

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