module: make require.cache reassignable

[bengl]

Checklist

• make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

module

Description of change

Previously, when wanting to clear require.cache completely, the only
options were to manually delete each property or to reassign the
underlying cache object at require('module')._cache.

By making require.cache a passthrough to require('module')._cache, the
require cache can now be cleared by just setting require.cache to a new
empty object.

[lpinca]

Any reason for not using assert.strictEqual(require(relativePath).extraProperty, undefined);?

[bengl]

Hmm, no. No reason. Fixed that.

[lpinca]

Is this semver minor? Also I'm not sure what is the policy for module changes.

[bengl]

@lpinca My guess is semver-minor at the very least.

It turns out require.cache does not have a stability index. While the change is in module, it doesn't affect how module operates, apart from require.cache.

[lpinca]

or to reassign the underlying cache object at require('module')._cache

If Module._cache is reassigned doesn't require.cache still hold the old object?

[jbergstroem]

CI: https://ci.nodejs.org/job/node-test-commit/5403/

[lpinca]

I would like to see a slightly better description as reassigning require('module')._cache does not clear require.cache.
Apart from this LGTM.

[ChALkeR]

Do we actually want to support this?

Also note that the example in the testcase here doesn't delete all the references to the original module (even the internal ones), and re-iterating that would eventually exhaust all the available memory and crash. See #8443.

If we do want to support actually clearing (or partially clearing) require cache, we need a better API for that, and that API should both work without major issues and be documented.

If we don't want to support that — I see no need in this change.

/cc @nodejs/ctc, perhaps.

[indutny]

I don't support this change. Locking into such internals-exposing API will potentially cause us troubles in the future, and the incentive for this change doesn't seem to overweight the cons for me.

[bengl]

@ChALkeR Some testing modules clear the require cache via require('module')._cache, (example), and the intent here is for them not to use an underscore-prefixed property. I don't think #8443 should be a blocker here, as clearing individual cached modules is supported anyway. I'm not sure that clearing items from require.cache should ever really be seen as a memory-freeing mechanism.

@indutny Do you mean in terms of changing require('module')._cache? It's already exposed as require.cache (but currently by assignment only) so I don't thinks this changes the impact much. Also, require('module')._cache is being used in the wild, so I don't think it can be easily changed anyway.

[indutny]

@bengl _cache is an underscored property any use of it is (of course) officially discouraged. Furthermore node.js has no API stability obligations for module._cache and it may be removed/replaced at any time in the future.

I wonder if there are any implications with the ongoing modules effort? cc @bmeck

[lpinca]

What happens to require.cache if Module._cache is removed?

[bengl]

@indutny Right, hence the attempt here to make an officially supported method of clearing the whole cache (apart from manually deleting all the properties), since it's being done anyway. On the other hand... clearing the whole cache can lead to things like #6160. But as long as that remains documented (and it is), then I think that's not a problem.

Here's a potential alternative: The docs could be updated to make it clearer that the object cannot simply be replaced, and what to do instead.

@lpinca At the moment, nothing. This PR would solve that too, via the getter.

[lpinca]

@bengl yeah my comment was more a way to emphasize that require.cache and Module._cache are the same thing so the underscored _cache is actually used as a public API.

[Fishrock123]

I'm with @indutny on this, we'll need a cache api for es modules though. That being said I'm not entirely against having a proper (but underscored) way to do this imo

[ChALkeR]

@bengl

the intent here is for them not to use an underscore-prefixed property

The underscore is just the messenger, the fact is that it's a part of unsupported and undocumented internal API.

Removing the underscore or even a simplier change like in this PR don't change that, and in fact make things worse — it encourages using unsupported internal API and brings in some more lines of code to support such behaviour, without proper documentation.

If we need that for internal use, then this change would be fine. If the intent here is to change an internal API so that it could be better used by external libraries, but still keeping it internal — please, no. If we can come up with situations when this API is really needed and if we want to export such API — we need to make it public and documented.

the attempt here to make an officially supported method of clearing the whole cache

This PR doesn't change anything about that being not supported officially — that would require docs change.

[bmeck]

The idempotency requirement of ESM means only the global cache for finding resolved specifiers can be cleared. Local cache of unresolved specifiers cannot be cleared.

[jasnell]

I'm +1 on exposing a public API for this but I agree that this may not be the right way. Rather than using a getter/setter, I'd rather see a function on Module that would allow us to hide the internals in the case it does need to change later on. Something like Module.clearCache() or Module.resetCache().

[italoacasas]

@jasnell @Fishrock123 @indutny @ChALkeR

Is this something that you guys want to discuss in a CTC meeting ?

[jasnell]

it's likely worth discussing

[jasnell]

This would need to be revisited in light of #10789

[fhinkel]

ping @bengl

[bengl]

Closing since I don't think this is the correct approach anyway.