Closed mgol closed 3 months ago
I can submit a PR with optional chaining for parent
but I'm not sure how to test it.
Where is the root bug?
override-require
’s fault? (Can we fix or migrate off of that?)at cjsLoader (node:internal/modules/esm/translators:356:17)
I’m pretty sure we don’t want to optional-chain on parent.filename
because having the filename is a pretty critical assumption of tons of code elsewhere, and that would cascade to needing optional chains everywhere, without fixing anything.
I think the main bug is in Danger JS. override-require
first calls isOverride
with the parameters it received, the incorrect assumption was just that isOverride
will receive a truthy parent
. For example, take this Node.js repl code:
https://github.com/nodejs/node/blob/925a464cb8bab007dfc0ac5856b76134cdfda1cc/lib/internal/main/repl.js#L26-L28
require('internal/modules/cjs/loader')
.Module
._load(process.env.NODE_REPL_EXTERNAL_MODULE, undefined, true);
The second parameter, parent
, is undefined
here.
The _load
core definition starts here:
https://github.com/nodejs/node/blob/925a464cb8bab007dfc0ac5856b76134cdfda1cc/lib/internal/modules/cjs/loader.js#L944-L955
/**
* Load a module from cache if it exists, otherwise create a new module instance.
* 1. If a module already exists in the cache: return its exports object.
* 2. If the module is native: call
* `BuiltinModule.prototype.compileForPublicLoader()` and return the exports.
* 3. Otherwise, create a new module for the file and save it to the cache.
* Then have it load the file contents before returning its exports object.
* @param {string} request Specifier of module to load via `require`
* @param {string} parent Absolute path of the module importing the child
* @param {boolean} isMain Whether the module is the main entry point
*/
Module._load = function(request, parent, isMain) {
It looks like some load requests don't have the parent
so any _load
override should account for that.
It's hard to find documentation here as the _load
method is private so there's no official contract. But you can figure it out by looking at how it's used in Node.js itself.
I’m pretty sure we don’t want to optional-chain on
parent.filename
because having the filename is a pretty critical assumption of tons of code elsewhere
Here it wouldn't cascade anywhere, just return false
from shouldUseGitHubOverride
. This is filename
from parent
, not name of a requested file.
cjsLoader (node:internal/modules/esm/translators:356:17)
is also good to look at - the following line:
https://github.com/nodejs/node/blob/925a464cb8bab007dfc0ac5856b76134cdfda1cc/lib/internal/modules/esm/translators.js#L359
is just:
CJSModule._load(filename);
Note the lack of the parent
parameter, leaving it undefined. CJSModule
here is just an alias of Module
:
https://github.com/nodejs/node/blob/925a464cb8bab007dfc0ac5856b76134cdfda1cc/lib/internal/modules/esm/translators.js#L43
I guess my question here is - for the kind of module requests that are missing a parent, is it OK to assume shouldUseGitHubOverride
may return false
? If not then required information would have to be fetched differently.
I think that's probably reasonable for it to be optionally chained and to not be usable outside of a cjs context.
I can't imagine github URL pathing is a particularly well used feature in comparison to using it with local files, on what I imagine will be an increasing amount of ESM contexts.
It'd be nice if we could detect that it was ESM and trying to do this somehow, and throw but it does look like getting the parent is tricky in ESM
for the kind of module requests that are missing a parent, is it OK to assume shouldUseGitHubOverride may return false?
Now that you've clarified things for me, I agree with you and @orta, I think this is fine for now. If we run into new issues we can address them as they come!
Feel free to tag me on any PR @mgol, thanks!
It took me a while as I was on paternity leave but here it is, @fbartho: #1430
Describe the bug We have a Danger JS check verifying Prettier formatting of changed files. That check is quite complex and runs through multiple files but at one point it does:
This worked fine until we upgraded to Prettier
3.1.1
which is published as a mixed ESM/CJS module now, but even the CJS version mostly just re-exports wrapped ESM APIs.After the upgrade, the Danger JS check fails with an error:
The error points to the
filename
access fromparent
in theif
in this piece of code:As it turns out,
parent
can be undefined. Going through the Prettier-specific part of the stack, it crashes on the first line of theloadJs
function:If I change
parent.filename
in thatshouldUseGitHubOverride
util to an optional accessparent?.filename
, the check passes.Going through the stack, the
shouldUseGitHubOverride
util is used by theoverride-require
package in its override of the_load
method of the'module'
Node.js module as theisOverride
check:It's worth noting that Node.js source of the
_load
method itself starts with:followed by a lot of code outside of this
if
soparent
clearly does not have to be defined.To Reproduce Steps to reproduce the behavior:
package.json
&app.js
files.danger@11.3.1
&prettier@3.1.1
to dependencies.Expected behavior
The check should not crash, especially in such a low-level way. I'm not sure what's the exact purpose of the
shouldUseGitHubOverride
function and whether its logic can be skipped whenparent
is undefined but if that's the case then checking forparent
truthiness would be a good fix.Screenshots If applicable, add screenshots to help explain your problem.
Your Environment
Additional context Add any other context about the problem here.