fix: use native require cache of loaded entries only

[pi0]

Resolves #346 (backport for v1: #349)

Node.js >= 22.12.0 introduced a mechanism to support require("esm-mod") however if any error happens in this stage, it leads to require.cache entries for sub-modules with invalid/unloaded state, making jiti wrongly pick them.

This change is safe to still allow circular CJS dependencies (which work by actually reusing cache entries in loading state) but since jiti has an additional per-require cache it works in tests. However, it makes the performance regression (and potentially more issues!) in case two parallel requests have the same deps.

[pi0]

@joyeecheung Do you think it is something that could be fixed in the next versions of Node.js to remove stalled ESM cache entries? (or perhaps expose symbol to detect ESM cache entries?)

[joyeecheung]

Do you think it is something that could be fixed in the next versions of Node.js to remove stalled ESM cache entries? (or perhaps expose symbol to detect ESM cache entries?)

Not quite sure what this means - do you have a repro?

[joyeecheung]

For the fixtures in the PR here, I tried this snippet:

try {
  require('./esm.js');
} catch {
  console.log(Object.keys(require.cache))
}

Which logs this from the main branch of Node.js, and is working as expected I think.

[
  '/Users/joyee/projects/node/test.cjs',
  '/Users/joyee/projects/node/fn.js'
]

[pi0]

From https://github.com/thecrypticace/node-22-jiti-bug, this is state of require.cache after the try/catch fails.

let result;
try {
  result = require("./files/esm.js");
} catch (err) {
  console.error(err);
  console.log(require.cache)
}

Is it normal that fn.js cache entry remains in require.cache with exports: {} and loaded: false + [Symbol(kIsCachedByESMLoader)]: true?

(note import fn from "./fn.js"; is valid one but second import file from "./file"; is invalid one which is not in cache as expected but fn.js entry is the cause of recent regression)

[Object: null prototype] {
  '/Users/pooya/tmp/node-22-jiti-bug/index.cjs': {
    id: '.',
    path: '/Users/pooya/tmp/node-22-jiti-bug',
    exports: {},
    filename: '/Users/pooya/tmp/node-22-jiti-bug/index.cjs',
    loaded: false,
    children: [],
    paths: [
      '/Users/pooya/tmp/node-22-jiti-bug/node_modules',
      '/Users/pooya/tmp/node_modules',
      '/Users/pooya/node_modules',
      '/Users/node_modules',
      '/node_modules'
    ],
    [Symbol(kIsMainSymbol)]: true,
    [Symbol(kIsCachedByESMLoader)]: false,
    [Symbol(kIsExecuting)]: true
  },
  '/Users/pooya/tmp/node-22-jiti-bug/files/fn.js': {
    id: '/Users/pooya/tmp/node-22-jiti-bug/files/fn.js',
    path: '/Users/pooya/tmp/node-22-jiti-bug/files',
    exports: {},
    filename: '/Users/pooya/tmp/node-22-jiti-bug/files/fn.js',
    loaded: false,
    children: [],
    paths: [
      '/Users/pooya/tmp/node-22-jiti-bug/files/node_modules',
      '/Users/pooya/tmp/node-22-jiti-bug/node_modules',
      '/Users/pooya/tmp/node_modules',
      '/Users/pooya/node_modules',
      '/Users/node_modules',
      '/node_modules'
    ],
    [Symbol(kIsCachedByESMLoader)]: true
  }
}

[joyeecheung]

Is it normal that fn.js cache entry remains in require.cache with empty exports and loaded: false?

AFAICT, it is valid, loaded loosely means "executed" i.e. to make sure the code inside the module should not be run again. It's fine for the cache to contain entries that are not loaded and a second require/import will pick up where it's left. There is no specific contract about what these states mean/how the cache is maintained, so it's basically "whatever state management that keeps it working" instead of having some sort of proper invariant. For performance this state combination might even be desirable, as it prevents having to load fn.js from disk again.

[pi0]

Thanks for confirming. Would it make sense to you that there is a state exposed to indicate this entry is in invalid/errored state + esm loader? Without this info, it is not clear to jiti the module is still in a loading state or not and Symbols are private. (this has performance benefit that jiti can reuse native cache entries in parallel state as currently does without breaking Node 22.12+

If yes, I can happily followup in a Node.js issue.

[pi0]

(merging PR as hotfix)

[joyeecheung]

Would it make sense to you that there is a state exposed to indicate this entry is in invalid/errored state + esm loader?

If the Node.js loader does not actually use this state to do anything and merely maintains it for external consumption, a state like this is likely to end up being invalid over time or incorrect in various edge cases. And currently Node.js does not need a state like this to do anything. Maybe exposing an undocumented state that's actually used internally as a temporary solution is okay - e.g perhaps exposing the existing state module[kIsExecuting] can help inferring whether the module is created but not yet compiled, or compiled but not yet executed.

https://github.com/nodejs/node/blob/b6c9dbe7e1fd208db20b6aac0d25a14e3fc5a456/lib/internal/modules/cjs/loader.js#L1734

Note that like most undocumented properties in the module, there would not be any stability guarantee around it, as we are trying to deprecate monkey patching of the CJS loaders internals and any customization needs to be dealt with by the public API https://nodejs.org/api/module.html#customization-hooks & nodejs/node#56241

[pi0]

Jiti is not monkey patching any internal loader stuff other than reusing require cache entries. kIsExecuting being exposed as a global symbol would be amazing BTW (we can use it as additional safety measure even if no guarantee on stability -- it is better than total blind state haha).

Thanks again for your time answering all this!

[joyeecheung]

Can you open an issue in the Node.js issue tracker with some examples about what kind of state management that you are trying to solve, and we can see if exposing something like module.isExecuting is worth it to be a stop-gap fix?

[pi0]

Surely adding to my todolist