module: remove underscore modules from builtinModules and isBuiltin

[dario-piotrowicz]

these changes remove modules starting with underscore (e.g. __http_agent) from Module#builtinModules and Module#isBuiltin since they are not properly documented as public. They can't easily be removed since many libraries use them, but their usage should be discouraged and they should not be exposed by such utilities

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/startup

[anonrig]

Cc @nodejs/TSC this is a breaking change and requires 2 approvals.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.22%. Comparing base (ffc1cf6) to head (fbeea6a).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57540      +/-   ##
==========================================
- Coverage   90.22%   90.22%   -0.01%     
==========================================
  Files         630      630              
  Lines      185149   185154       +5     
  Branches    36236    36246      +10     
==========================================
- Hits       167058   167057       -1     
- Misses      11035    11045      +10     
+ Partials     7056     7052       -4     

Files with missing lines	Coverage Δ
lib/internal/bootstrap/realm.js	97.00% <100.00%> (+1.00%)	⬆️
lib/internal/modules/cjs/loader.js	98.26% <100.00%> (ø)

... and 23 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[RaisinTen]

Thanks for working on the PR!

Here are my thoughts:

The doc for module.builtinModules says:

A list of the names of all modules provided by Node.js. Can be used to verify if a module is maintained by a third party or not.

Removing these undocumented module names from this list would not change the fact that these are still builtin modules provided by Node.js and that these are not maintained by a third party.

Making this change would break code that tries to test if some module is builtin or not. Note that the said user code is not to blame here necessarily as that might not be the one using the undocumented module. This change would invalidate the doc and make the purpose of this API questionable.

For code that is already using these undocumented modules - they are doing it at their own risk because they are using an undocumented API. Changing the behavior of this API is not gonna stop them from continuing using these undocumented modules.

If usage of these undocumented modules poses a problem, I think it should be approached differently and changing the behavior of this API is not the right starting point. I think this would have been a sensible thing to do if there were some ongoing efforts of removing these undocumented modules but that's not where we are today.

So I don't think we should do this.

[ljharb]

Anything that's requireable/importable should be in this list.

[anonrig]

cc @nodejs/tsc @BridgeAR for visibility.

[dario-piotrowicz]

I very well understand your reasoning @RaisinTen. I agree that we have to keep such functionality around one way or the other. I do not believe we are able to stop exporting these "private" modules (while we might be able to at least add a pending deprecation).

That aside: what about having one breaking change: we stop returning the values by default and add an option to show the underscore ones as well? It is a breaking change that likely not many will run into, so fixing these cases might be fine?

what about the comprimise of remove the modules from Module.builtinModules and keep it in isBuiltin for now?

In that case isBuiltin can still be used to check if a module is acutally being provided by Node.js but those modules are not exposed to the same degree to users?

[dario-piotrowicz]

I very well understand your reasoning @RaisinTen. I agree that we have to keep such functionality around one way or the other. I do not believe we are able to stop exporting these "private" modules (while we might be able to at least add a pending deprecation).
That aside: what about having one breaking change: we stop returning the values by default and add an option to show the underscore ones as well? It is a breaking change that likely not many will run into, so fixing these cases might be fine?

what about the comprimise of remove the modules from Module.builtinModules and keep it in isBuiltin for now?

In that case isBuiltin can still be used to check if a module is acutally being provided by Node.js but those modules are not exposed to the same degree to users?

An a flag to isBuiltin could be added not to include these modules? (e.g. excludeUndocumetedModules?)

[ljharb]

@dario-piotrowicz currently i rely on being able to enumerate the entire list in the tests for resolve. I think that if we don't want to expose them to users it's incumbent on us to stop making them available, rather than just pretending that the list is "cleaner".

[dario-piotrowicz]

@dario-piotrowicz currently i rely on being able to enumerate the entire list in the tests for resolve. I think that if we don't want to expose them to users it's incumbent on us to stop making them available, rather than just pretending that the list is "cleaner".

I see... 😓

[mcollina]

lgtm

[ExE-Boss]

Note that, as currently implemented, isBuiltin(…) will return true for node:_http_common and others, even though node:nonexistent and node:internal/util both return false.

[dario-piotrowicz]

@ExE-Boss good call! I've fixed that, thanks! 😄

[ljharb]

I continue to not see the value in this change - the modules’ presence in this list doesn’t encourage their use, and if the list omits them then userland has to manually maintain its own list to the same effect.

[dario-piotrowicz]

I continue to not see the value in this change - the modules’ presence in this list doesn’t encourage their use, and if the list omits them then userland has to manually maintain its own list to the same effect.

@ljharb just to be clear, I wasn't making my last change in order to disregard your comments, I just figured why not keep the PR tidy and clean until a decision is made 🙂

if the list omits them then userland has to manually maintain its own list to the same effect

Doesn't this mean that this would introduce inconvenience to users relying on these modules? So maybe it could be seen as positive that this adds some friction in this sense? 🙂

out of curiosity, do you imagine ever possible for these modules to actually not be exposed? (or is that a too big of an effort?)

[ljharb]

No, only an inconvenience to tooling that needs to enumerate builtin modules, like resolve, is-core-module, and eslint-plugin-import.

I don’t know how much they’re used in modern code, but a good start would be runtime-deprecating them.

[dario-piotrowicz]

No, only an inconvenience to tooling that needs to enumerate builtin modules, like resolve, is-core-module, and eslint-plugin-import.

I see 😓

but a good start would be runtime-deprecating them.

Sounds interesting, do you think that's something I could help out with?

Regarding this PR and your pushback I don't really feel like I'm in a position to argue too strongly about this as I am not super familiar with these modules and their use to begin with (I just find it pretty messy and unclean for there to be internal non-public modules exposed as public)

[ljharb]

They’re reachable, so they’re public :-)

[dario-piotrowicz]

They’re reachable, so they’re public :-)

not according to the docs! 😜 🥲

[ljharb]

The docs try to document reality; they don't define it :-)

[dario-piotrowicz]

Given the (reasonable) pushback from @ljharb and @RaisinTen I feel like this might be too contentious to purse so I'm closing the PR

Thanks y'all for the reviews anyways 🙂

[BridgeAR]

@dario-piotrowicz thanks for looking into this anyway!

What @ljharb and @RaisinTen says is absolutely correct. This actually made me look into the usage of these modules from a github search for the stream and tls based entries, it seems that these are pretty much only used when the Node.js code base was forked at some point. I could not find a single legit usage. So actually deprecating the runtime usage is probably a good idea (I would skip the documentation runtime, due to it's limited use in the first place). We could split the deprecation into three parts or something like that, where we deprecate tls, stream, and http related underscore modules and check CITGM as well.
While looking through the docs, I actually noticed that one of these is already deprecated (_stream_wrap).

This would also resolve the problem that these modules are not mentioned in our docs.