module: expose module.format

[joyeecheung]

To help users relying on require.cache determine the format of a cached module.

Refs: #59868

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[GeoffreyBooth]

I'm concerned that exposing this means that minor changes to the flow, like as we work on hooks, suddenly become breaking changes because the format might be known at a different point in the process; or it's defined as one thing at one point and then reassigned later, and the point of reassignment becomes a breaking change.

I think users can already get this value via the hooks; what's the use case for making it easier?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.30%. Comparing base (58f408f) to head (ad8f4cd).
⚠️ Report is 711 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #59904      +/-   ##
==========================================
+ Coverage   88.29%   88.30%   +0.01%     
==========================================
  Files         702      702              
  Lines      206875   206919      +44     
  Branches    39797    39815      +18     
==========================================
+ Hits       182655   182725      +70     
+ Misses      16226    16213      -13     
+ Partials     7994     7981      -13     

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	98.13% <100.00%> (+0.20%)	⬆️

... and 33 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.

[joyeecheung]

I think users can already get this value via the hooks; what's the use case for making it easier?

See #59868 - Webpack is directly checking entries from the require.cache and the children. (note that they support all the way to Node.js 10)

suddenly become breaking changes because the format might be known at a different point in the process

I am somewhat hesitant about it because of this as well though I think the docs should make it clear enough:

This property is not guaranteed to be defined on the module object, nor to be accurate. If
it's undefined, it's likely that Node.js does not yet know the module format.

[ljharb]

doesn't this make "changing the format of a module" always a breaking change, which it needn't be at the moment?

[joyeecheung]

doesn't this make "changing the format of a module" always a breaking change, which it needn't be at the moment?

In what circumstance would we change the format of the module?

Note that the documentation is making it clear that it's not going to be accurate, or always defined. I think if that's defined to be breaking then changing it would already break hooks.

[GeoffreyBooth]

I am somewhat hesitant about it because of this as well though I think the docs should make it clear enough:

This property is not guaranteed to be defined on the module object, nor to be accurate. If
it's undefined, it's likely that Node.js does not yet know the module format.

This is basically a way of saying “we’re exposing a bit of Node internals here, and you shouldn’t expect us to honor semver with regards to the behavior of this internal feature.” But in reality, are people really going to respect that? When Webpack opens a bug issue because some minor change in the module loader breaks something on their side because of how we treat format internally, are we just going to point to this line in the docs and say “sorry, you relied on internals too much, the docs said not to”? If we already feel like we can’t take such a hard line on monkey-patching, which isn’t even documented, then why would we be able to hold the line here?

Or alternatively, maybe we expose module.format only when --expose-internals is passed? Then it’s a lot more explicit what’s happening here, and the contract that the consumer is opting into by relying on this.

[joyeecheung]

are we just going to point to this line in the docs and say “sorry, you relied on internals too much, the docs said not to”?

I think yes, it's inevitable that we will have to do this for the internals until people actually migrate off using them. As always the docs just discourage people from using them, but when there's no practical alternative for them because of their own legacy, which I think is happening in the case of Webpack (from a brief look of what they are doing I am skeptical whether "migrating to use the experimental hooks which have their own quirks and are only available since v20" is a viable alternative, but maybe @alexander-akait knows more), then making the legacy a bit more practical for them in the meantime is what it takes for a good overall end UX. I think "we are working on a better experimental alternative" isn't really a practical response for "there are code being broken right now", what would be more practical would be "we are working on a better experimental alternative, but at the mean time, use this this band-aid to at least regress fewer end users".

Or alternatively, maybe we expose module.format only when --expose-internals is passed?

I think that'll incur a breaking change for the packages that are already relying on require.cache entries for detection but are not necessarily in control of the CLI arguments (which are controlled by end users).

[ljharb]

People rewrite a module from CJS into ESM all the time, and potentially back to CJS as well. Refactoring shouldn't be forced to be a breaking change.

[joyeecheung]

If that was considered a breaking change, then it is already breaking even if this property isn't exposed, because that format change would be visible in the loader hooks already. It doesn't seem exposing it in one more path (which actually covers less) would make that much of a difference. Though I'd say just like results and contexts in loader hooks, those who access it should not assume one particular module must be in a certain format because that can always be customised by somebody else.

[JakobJingleheimer]

🙌

This doesn't expose it for ESM, right, only CJS modules?

[JakobJingleheimer]

"nor be accurate". Maybe a little bit more on why and why a user might want to use this anyway?

[joyeecheung]

This doesn't expose it for ESM, right, only CJS modules?

This exposes it for any module that's been directly required and is therefore in the require.cache - including ESM, but not if it's never loaded directly by require(), the inner modules won't be in require.cache either until they are directly required. This is to address the breakage for Webpack and similar tools that are looking into modules in the cache. #59868

[ljharb]

@joyeecheung loader hooks don't count, because that's node-level superpowers. In a loaderless node application, it's not currently a breaking change, and this makes it one (unless this information was restricted to loader hooks, ofc)

[joyeecheung]

loader hooks don't count, because that's node-level superpowers.

Why are loader hooks node-level superpowers but require.cache isn't? In practice people operate on Module instances in very similar ways that they use the loader hooks in, or it's common to implement hooks by hijacking Module instances too. If property change on the Module instance is considered to be a breaking change then there can be many more breaking changes - for example by refactoring it into ESM it might disappear from require.cache altogether, or the filename could be changed, or its parent or children properties would also change. It's unlikely that format would be the only property that's changed.

In a loaderless node application, it's not currently a breaking change.

In an application that doesn't access the require.cache, it's not a breaking change either. I think they are very much in the same boat?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/70617/