Node: Hide private symbols

[BridgeAR]

Right now we use a lot of "private" symbols in core. However, those show up when using console.log or util.inspect (the latter as the actual part that makes them visible). The reason for that is that we add them as enumerable symbols instead of non-enumerable ones.

The default for util.inspect is to show all enumerable properties. I would like to hide all of these by changing our symbols to non-enumerable properties. That way they could still be inspected when calling inspect with the showHidden option but I believe that is exactly as it should be.

Are other @nodejs/collaborators fine with changing all those entries? We would only have to change it in the places where we actually add the symbol to the object.

If that is something we all agree to, I would like to add the good-first-issue and help-wanted labels and we should pay attention to always make sure they are added as non-enumerable properties and add tests for that as well to prevent regressions.

[addaleax]

I don’t think we should make that move unconditionally for every symbol. In particular, showing their values in util.inspect is something I consider helpful in general if it provides extra diagnostic information about the object’s state.

[jdalton]

@addaleax Couldn't you use showHidden still?

[addaleax]

@jdalton Yes, you can, but it’s extra overhead. If we decide to do this, we should have a clear understanding of why we get better debugging output when they are excluded.

[BridgeAR]

@addaleax I agree that it might help for debugging but that is what showHidden is for out of my perspective. It should not be visible to all users unconditionally when they just call console.log on an object. I also believe that the repl should not print these by default but only when explicitly asked for.

[addaleax]

@BridgeAR It might be helpful if you could explain why you believe that? From my perspective, it seems like more information about an object’s state is not a bad thing when coming from a diagnostic utility.

[BridgeAR]

I believe the repl and console.log output are main points where people get the idea to use private internal things directly instead of relying on the public API (for example #19671) . I doubt that people always look into the code to to do these things and instead check what's available on the objects returned from core. That is only speculation and it is no guarantee for anything by hiding them but I just do not think the symbols should be exposed by default.

[devsnek]

seeing as v8 extras isn't deprecated anymore, we could also replace them all with private symbols. it would completely kill debugging though.

[addaleax]

I think in that case we might just want to wait until V8 supports private properties à la https://github.com/tc39/proposal-class-fields? It’s stage 3, so I’d assume it’s not so terrible long until we have it available.

Otherwise we’re just shifting the same code twice, right?

[BridgeAR]

@addaleax that might be a solution to most cases but we could tackle this right now. As I said above I think those would be good as good first issues.

[refack]

IMHO the crux is that util.inspect(coreAPIObj) and console.log(coreAPIObj) should have independent outputs (the first is a debug tool, the second a common runtime tool).

I'm now thinking we should have a different inspect flavor that is more debug-like (maybe even available only behind a CLI flag). So that we can un-link util.inspect and console.log.

[targos]

IMHO the crux is that util.inspect(coreAPIObj) and console.log(coreAPIObj) should have independent outputs (the first is a debug tool, the second a common runtime tool).

Interesting. My opinion is the other way (console.log for debugging, forbidden with linter).

[refack]

Interesting. My opinion is the other way (console.log for debugging, forbidden with linter).

I agree with you RE the code in core. But we can't enforce that for code in the wild (where console.log is is affected by changes in util.inspect semantic)

[targos]

I'm talking about code in the wild (my team's modules and apps). We use console.log(obj) for debugging, and sometimes console.log(util.inspect(obj, opts)). IMO, the more data we have with the default inspection used by console.log, the better.

[refack]

I'm talking about code in the wild (my team's modules and apps). We use console.log(obj) for debugging

So another possibility is the create "module scope globals" and implement require('util').inspect.defaultOptions so it's stored at that scope.

As for linting console.log from production code in the wild, that IMHO is a hopeless battle ;)

The docs have several util.inspect disclaimers:

but the console.log API is unqualified so essentially we can't change the semantics of console.log('%o', ...).

I've seen discussions around this pop up every couple on months and I'd say we should make a formal decision either way.

[Trott]

Should this be closed given stalling/inactivity? It can always be re-opened if it becomes A Thing again.

[Trott]

This has been stalled since late May, so I'm going to close it. Feel absolutely free to re-open it or comment. I'm just tidying up and not acting on a super-strong opinion or anything like that.