Node: module.children behavior does not match documentation (only tracks first requires)

[codebling]

TL;DR module.children does not behave according to documentation.

What be the expected behaviour of module.children?

Documentations states that module.children is "the module objects required by this one." This is false, as it only lists the children that __were required for the first time__ by the module. Since module.parent is correctly documented as containing "the module that first required this one", it seems like this may be a documentation bug. If that's the case, we should correct the documentation.

Considering the issue on another level, what is the purpose of module.children and module.parent? Having a full graph of the first requires, or a partial/incomplete graph of all require relationships, does not seem at all useful to me. Why do these exist? I propose the addition of module fields requirers and requirees as respective analogues of parent and children, but ones which will keep track of all requires. The code will be essentially the same as what is currently in place, except that instead of running on module initialisation and pushing this module into the parent module's children structure (which only works the first time it is required, since after that the module is initialised and cached), we run after every require, pushing this module into the child's requirers. This will generally also only run once per uncached module (except for modules with conditional or lazy requires), except still works even if the children are cached.

So, fix options here are:
1) consider this a documentation bug and fix the documentation
2) change the behaviour of module.children to match the documentation (fix basically described above)

Possibly related to #7131 and #3933

[codebling]

Note: I did not (perhaps rudely) fill out the template with version/platform information, since this affects multiple versions and is probably a documentation bug.

Testing this is trivial, however here is a simple example showing the issue.

[tniessen]

This is definitely a good point. I am even more concerned about the fact that modules are not cached if they throw an Error while being require()d, but they are being added to module.children:

try {
  require('./b.js');
} catch(err) {
  try {
    require('./b.js');
  } catch(err) {
  }
}

console.log(`There are ${module.children.length} child modules`);
for(let child of module.children) {
  if(!require.cache[child.id])
    console.log(`${child.id} is not in require.cache!`);
}

This prints something like

There are 2 child modules
E:\node\testing\b.js is not in require.cache!
E:\node\testing\b.js is not in require.cache!

Is this intended behavior? (See also #13386 for a similar topic.)

Which solution would we prefer?

1. Update the documentation to clearly state that only the first require()-call will add the module to module.children (except if an Error occurred).
2. Change the implementation to add the module to require.children each time require() is used.

Edit: @bmeck Just made me realize that adding a module as the child of multiple other modules would be inconsistent as well as each module only has one parent module. Or, to put it with his words, "i would say nothing is consistent about module.parent or module.children".

[codebling]

@tniessen the more useful approach to me is to deprecate module.parent and module.children and create other fields as I've described to address the multiple issues with module.parent and module.children (only first requires are tracked, added to children despite Error, etc). @bmeck is correct, that's why I suggested requirers and requirees.

It's not that much work to fix this, I think I even had a patch kicking around at some point. It's useful for introspection/reflection type behaviours.

[Trott]

@nodejs/modules Anything happening with this? Is this still an issue? Is this something that might benefit from a help wanted label?

[benjamingr]

@Trott the modules team is tasked with ESM integration into Node.js - I think this is something that is out of scope for us - if this is stalled then I think help wanted is appropriate.

[Trott]

@benjamingr Sorry about that! I assumed nodejs/modules was the module subsystem and there was a nodejs/es-modules team or something but it looks like I was wrong.

[ljharb]

I think this is definitely in scope for the modules group, because anything supported in CJS has to be explicitly supported, or not, in ESM.

In this case, runtime reflection on the module graph is not a feature I think node should support in either CJS or ESM, so I'd be strongly in favor of deprecating it in CJS.

[benjamingr]

@ljharb I agree in general though the implementation in this case (module.children) doesn't really have much to do with how esm/cjs interacts in particular.

[ryzokuken]

Couldn't we just make a change in the docs? The behavior seems pretty normal, feel free to change it if you want, but I believe we should just tag this as a "good first issue" with a single doc change.

[benjamingr]

Taking @ryzokuken's advice: if anyone wants to tackle this as a first issue and fix the docs - please let us know if you want/need help :)