[BUG] prettier.resolveConfig fails due to faulty shouldUseGitHubOverride

[mgol]

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:

const fileInfoOptions = {
  ...(await prettier.resolveConfig(filePath))
};

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:

TypeError: Cannot read properties of undefined (reading 'filename')
    at shouldUseGitHubOverride (my-project/node_modules/danger/distribution/platforms/github/customGitHubRequire.js:131:16)
    at _module2.default._load (my-project/node_modules/override-require/dist/overrideRequire.js:38:9)
    at cjsLoader (node:internal/modules/esm/translators:356:17)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/translators:305:7)
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async loadJs (file://my-project/node_modules/prettier/index.mjs:22668:18)
    at async loadConfig (file://my-project/node_modules/prettier/index.mjs:22726:16)
    at async loadPrettierConfig (file://my-project/node_modules/prettier/index.mjs:23032:18)
    at async Promise.all (index 0)

The error points to the filename access from parent in the if in this piece of code:

var shouldUseGitHubOverride = function (request, parent) {
    // Is it a from a file we're handling, and is it relative?
    if (parent.filename.startsWith(exports.dangerPrefix) && request.startsWith(".")) {
        return true;
    }
    // Basically any import that's not a relative import from a Dangerfile
    return false;
};

As it turns out, parent can be undefined. Going through the Prettier-specific part of the stack, it crashes on the first line of the loadJs function:

async function loadJs(file) {
  const module = await import(pathToFileURL4(file).href);
  return module.default;
}

If I change parent.filename in that shouldUseGitHubOverride util to an optional access parent?.filename, the check passes.

Going through the stack, the shouldUseGitHubOverride util is used by the override-require package in its override of the _load method of the 'module' Node.js module as the isOverride check:

  _module2.default._load = function (request /*: string*/, parent /*: ParentType*/) {
    if (isOverride(request, parent)) {
      return resolveOverride(request, parent);
    }

    // eslint-disable-next-line prefer-rest-params
    return originalLoad.apply(this, arguments);
  };

It's worth noting that Node.js source of the _load method itself starts with:

function(request, parent, isMain) {
  let relResolveCacheIdentifier;
  if (parent) {

followed by a lot of code outside of this if so parent clearly does not have to be defined.

To Reproduce
Steps to reproduce the behavior:

1. Create a repository with package.json & app.js files.
2. Add [email protected] & [email protected] to dependencies.
3. Write a Danger JS check which does the following internally:

const prettier = require('prettier');
await prettier.resolveConfig('app.js')

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 when parent is undefined but if that's the case then checking for parent truthiness would be a good fix.

Screenshots
If applicable, add screenshots to help explain your problem.

Your Environment

software	version
danger.js	11.3.1
node	v20.11.0
npm	10.2.4, but we use Yarn 1.22.18
Operating System	macOS Sonoma 14.3

Additional context
Add any other context about the problem here.

[mgol]

I can submit a PR with optional chaining for parent but I'm not sure how to test it.

[fbartho]

Where is the root bug?

• Is it override-require’s fault? (Can we fix or migrate off of that?)
• Or is it something in at cjsLoader (node:internal/modules/esm/translators:356:17)
• Or what?

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.

[mgol]

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.

[mgol]

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

[mgol]

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.

[orta]

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

[fbartho]

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!

[mgol]

It took me a while as I was on paternity leave but here it is, @fbartho: #1430