module: rebase clearCache and address review comments - #27
Conversation
Document required parentURL/resolver, non-absolute specifier examples, and that clearCache does not follow user-module links or recurse. Reject URL objects when resolver is require, drop linked ModuleWrap references after successful instantiation, and stop testing internals in the clearCache suite. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Yagiz Nizipli <anonrig@users.noreply.github.com>
Remove an unused eslint-disable in the reload example and place the ECMA-262 heading reference in alphabetical order. Signed-off-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Yagiz Nizipli <anonrig@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6b903ee. Configure here.
|
|
||
| const parent = parentPath ? createParentModuleForClearCache(parentPath) : null; | ||
| try { | ||
| const { filename, format } = resolveForCJSWithHooks(specifier, parent, false, false); |
There was a problem hiding this comment.
Require clearCache silently no-ops
High Severity
resolveForCJSWithHooks is called with false as the fourth argument, but that parameter is now a destructured options object. Destructuring false throws, and the surrounding catch turns that into null, so every resolver: 'require' clearCache call silently skips clearing CJS and related caches.
Reviewed by Cursor Bugbot for commit 6b903ee. Configure here.
| // Re-install so wrapModuleLoad finds the same module instance. | ||
| if (CJSModule._cache[filename] !== module) { | ||
| CJSModule._cache[filename] = module; | ||
| } |
There was a problem hiding this comment.
Race overwrites newer CJS cache
Medium Severity
The in-flight import() race fix reassigns Module._cache[filename] whenever it is not the current module, including when a newer post-clearCache load already owns that slot. Concurrent clear-then-reimport can clobber the fresh cache entry and produce split or flaky module instances.
Reviewed by Cursor Bugbot for commit 6b903ee. Configure here.
2 participants
Updates the
module.clearCachework from nodejs/node#61767 after rebasing onto currentmainand addressing the remaining review threads.Rebase
Rebased
yagiz/node-module-clear-cacheontoorigin/main. Conflicts were in:lib/internal/modules/esm/translators.js— kept main'sloadCJSModuleWithModuleLoad(translateContext, hook skip, patched_loadexport mirroring) and re-installed the CJS cache entry whenclearCacheraces an in-flightimport().lib/internal/modules/helpers.js— exported bothgetRequireStack(from main) andgetFilePathFromFileURL(from this change).The original PR branch
yagiz/node-module-clear-cachewas force-pushed with the same commits so nodejs/node#61767 is updated.Review comments addressed
Unresolved threads from joyeecheung, jsumners-nr, and related conversation comments:
parentURLandresolverare documented as required. Added relative, bare, and absolute-URL examples. Clarified there is no recursive option. Rewrote the memory-retention section so it talks about user-module references (for example staticimport) rather thanModuleWrap/ V8 internals. Added a CommonJS test-reload example.resolver: 'require'+URL: throwsERR_INVALID_ARG_TYPE. String specifiers that look likefile:URLs are still treated as paths, matchingrequire().kLinkedRequestsSlot: cleared after successfulModuleWrapinstantiation so imported wraps are not pinned after they leave Node.js caches.hash_to_module_map.Recursive cache clearing was not added (see laverdet's comment on that thread).
Testing
ESLint on the changed JS/test files and
doc/api/module.mdis clean.lint-mdondoc/api/module.mdis clean. A full Node.js rebuild is still needed to run thetest/parallel/test-module-clear-cache*andtest/es-module/test-module-clear-cache*suites, and to exercise the C++kLinkedRequestsSlotchange.