@oclif/core cannot load index.js files when using ES modules

[rpastro]

I started converting my @oclif/core project from Common.js to ES modules, but unfortunately I reached a blocking point. It seems @oclif/core cannot load any of the index.js files under the commands folder. When running the CLI it prints the following error:

(node:49740) [ERR_REQUIRE_ESM] Error Plugin: @ngls/cli [ERR_REQUIRE_ESM]: require() of ES Module /Users/rpastro/ngls/ngls/cli/src/commands/elements/index.js from /Users/rpastro/ngls/ngls/cli/node_modules/@oclif/core/lib/module-loader.js not supported.
Instead change the require of index.js in /Users/rpastro/ngls/ngls/cli/node_modules/@oclif/core/lib/module-loader.js to a dynamic import() which is available in all CommonJS modules.
module: @oclif/core@1.0.10
task: toCached
plugin: @ngls/cli
root: /Users/rpastro/ngls/ngls/cli

All other files are successfully loaded by @oclif/core. The problem is only with the index.js files.

Looking into the code, the problem seems to be in the ModuleLoader.resolvePath function. The modulePath parameter passed in case of index.js is the folder name. For instance, if the command file is ".../commands/users/index.js", modulePath is set to ".../commands/users". Since this is a valid directory, fs.existsSync(filePath) returns true and ModuleLoader.isPathModule returns false since there is no extension in ".../commands/users" and we fall into the default branch.

[typhonrt]

I worked on the ESM support ~March - May as an outside contributor, so just giving some basic info given your error messages above. It appears you haven't added "type": "module" to your package.json or you are not working with *.mjs files. The error message indicates that you are working with a mixed codebase IE require() of ES Module. Technically you can do that, but you have to explicitly use *.cjs for CommonJS files and *.mjs for ESM, so that Node knows which type of source you are using.

Though possibly you have found an edge case given the second half of your message. There is fairly comprehensive test coverage. I've been meaning to check back in with the Oclif project and make sure the ESM support I contributed is in tip top shape. Quite possibly I can work up a test case with info you have provided. the existsSync check along with making sure it's a file and not a directory might be pertinent, but as mentioned would like to put together a test case that triggers this potential issue.

[rpastro]

@typhonrt Thanks for the feedback. I do have "type": "module" in my package.json and all my files have a *.js extension. The error message is not complaining about a require in my code. The error indicates that module-loader.js is loading the commands/elements/index.js file in my project using require.

I have a submitted a pull request that address the issue. See #320

[rpastro]

The problem is not exclusive to index.js files.

Suppose the CLI needs to support the following commands: (users, users:add, users:remove, users:update). Neither of the scenarios below work at the moment when using ES modules.

1. users command is in "commands/users.js" file. module-loader.js tries to load "commands/users" using require instead of import.

   commands
   +-- users.js
   +-- users
       +-- add.js
       +-- remove.js
       +-- update.js

2. users command is in "commands/users/index.js" file. module-loader.js tries to load "commands/users" using require instead of import.

commands
    +-- users
        +-- add.js
        +-- index.js
        +-- remove.js
        +-- update.js

[typhonrt]

@rpastro I'm glad you are digging into the aspects I didn't catch in the initial implementation for ESM! Perhaps coordinate with @amphro who is on the core team regarding getting pull request / patches in for ModuleLoader. Alas, I'm busy with other development efforts presently; IE I'm not on the core Oclif dev team.

I can attempt to point out where you need to add tests and help review the changes.

[rpastro]

@peternhale Any chance someone from core team can take a look at this issue or the pull request #320 ?

[gimenete]

I've opened https://github.com/oclif/core/pull/417 branched off #320 and adding tests to it.

Also, if somebody is interested, I've been able to monkey-patch oclif with the code from the PR (with a couple of minor changes):

cli/bin/run.js

#!/usr/bin/env node
import { Errors, flush, run } from '@oclif/core';
import './mokey-patch-oclif.js';

run(void 0, import.meta.url)
  .then(flush)
  .catch(Errors.handle);

cli/bin/mokey-patch-oclif.js

/**
 * Oclif mostly supports ESM, but it doesn't properly support commands
 * in "index.js" files. It always thinks they are not ESM files.
 *
 * This file monkey-patches Oclif with the code
 * from this PR https://github.com/oclif/core/pull/417 to support them.
 */
import { tsPath } from '@oclif/core';
import ModuleLoader from '@oclif/core/lib/module-loader.js';
import fs from 'fs';
import path from 'path';

ModuleLoader.default.resolvePath = (config, modulePath) => {
  let isESM
  let filePath

  try {
    filePath = require.resolve(modulePath)
    isESM = ModuleLoader.default.isPathModule(filePath)
  } catch {
    filePath = tsPath(config.root, modulePath)

    let fileExists = false
    let isDirectory = false
    if (fs.existsSync(filePath)) {
      fileExists = true
      try {
        if (fs.lstatSync(filePath)?.isDirectory?.()) {
          fileExists = false
          isDirectory = true
        }
      } catch {
        // ignore
      }
    }

    if (!fileExists) {
      // Try all supported extensions.
      let foundPath = findFile(filePath)
      if (!foundPath && isDirectory) {
        // Since filePath is a directory, try looking for index.js file.
        foundPath = findFile(path.join(filePath, 'index'))
      }

      if (foundPath) {
        filePath = foundPath
      }
    }

    isESM = ModuleLoader.default.isPathModule(filePath)
  }

  return {isESM, filePath}
}

function findFile(filePath) {
  // eslint-disable-next-line camelcase
  for (const extension of ['.js', '.cjs']) {
    const testPath = `${filePath}${extension}`

    if (fs.existsSync(testPath)) {
      return testPath
    }
  }

  return null
}

[gimenete]

This PR has been merged https://github.com/oclif/core/pull/417 so this issue should be fixed in the next release.

[rpastro]

Fixed by #417