Modules: Named Exports for builtin modules

[benjamingr]

This is more of a technical discussion than an API discussion since it doesn't deal with universal code or ESM/cjs interop but rather with how named builtin modules are exported using named exports.

@devsnek has went ahead and worked on https://github.com/nodejs/node/pull/18131 for adding named exports for builtins. It is currently blocked on https://github.com/nodejs/node/pull/18131#issuecomment-362011457

I think we should either:

• expose another (internal) symbol for things Node wants to hide from inspect and put it specifically on proxies we use for live bindings
• modify customInspect to run before showProxy
• Don't wrap the live bindings in proxies, start with explicitly not supporting it in named exports and move to setters/getters slowly.

I'm willing to work on either one - pinging involved parties for discussion @addaleax @jdalton @devsnek @TimothyGu @BridgeAR @ljharb

I think the first solution is the easiest - opinions?

[jdalton]

expose another (internal) symbol for things Node wants to hide from inspect and put it specifically on proxies we use for live bindings

The proxy bit with inspect was already handled in the PR

modify customInspect to run before showProxy

👆 (already handled)

Don't wrap the live bindings in proxies, start with explicitly not supporting it in named exports and move to setters/getters slowly.

This is just an issue with a console.log reference. It isn't === what is expected which means it's likely a reference that is being gotten too early before being bound or rebound.

[devsnek]

I was under the impression the only thing blocking this was the extra stack frame in inspector from moving the method to js. the pr already overrides showProxy

[jdalton]

I was under the impression the only thing blocking this was the extra stack frame in inspector from moving the method to js. the pr already overrides showProxy

Yep. I think it can be resolved by simply finding the console.log reference issue and resolving it. That avoids reimplementing log in JS and the stack frame issue.

[benjamingr]

Got it, thanks, so no decision to be made - just a tech issue

[jdalton]

Correct. With some more debugging I'm sure the root cause can be found and resolved.

[ljharb]

Can the changes to inspect be pulled out and merged separately? It seems important, regardless, to allow a custom inspect method to know if it's running on a proxy or not.

[devsnek]

the change is if (showProxy) { to if (showProxy && !_nsCache.has(value)) {, i'm not sure its really large enough to be put in its own pr

[ljharb]

I think it’s important enough that it shouldn’t be held up by discussions on module changes.

[MylesBorins]

In todays meeting we had consensus that there is no reason to request core to hold off on implementing this.

[benjamingr]

@MylesBorins this is stuck on a technical issue though 😅

I'm leaving this open to discuss the technical aspects and since discussion was still happening - no strong feelings about this being open though.

[devsnek]

isn't the original pr an ok place to discuss the technical stuff?

[benjamingr]

@devsnek I guess so - up to you where you want the discussion to be had, here is's more visible to the entire modules team, on the actual PR it's more like the regular issue tracker process.

[MylesBorins]

I personally think that it makes sense to have the conversation in the main repo. We can do an update at a future meeting on the state of the implementation, from my perspective the team is currently focused on higher level concepts