Integrate ESM loading of commands & hooks #160

typhonrt · 2021-05-07T04:22:00Z

Greets. This is the first of three PRs regarding adding ESM support to Oclif v2.

This initial PR adds ESM loading of commands and hooks. I have tested it with my ESM Oclif middleware / CLI tool in addition to all tests passing in the repo.

The easiest way for me to proceed with working on the Help subsystem adding ESM help class loading support and async handling is for this PR to be merged first as it depends on the ModuleLoader addition.

Let me know your thoughts @amphro.

amphro · 2021-05-07T05:13:39Z

src/config/plugin.ts

      let m
      try {
-        m = require(p)
+        m = isESM ? await ModuleLoader.importDynamic(p) : require(p)


Can you call ModuleLoader.load(this, p) since that has the importDynamic/require logic?

Both load and loadWithData pass the string through ModuleLoader.resolvePaths

This require.resolve line (require.resolve(path.join(this.commandsDir, ...id.split(':')))) uses this.commandDir
https://github.com/oclif/core/pull/160/files/28566d746b4ec5b0d8be3ef2ba5d7808d213452d#diff-0edcef2f1da996cde64784a30d76043fbe58fb9352f6b0189e1d135b868ee8f1L184

In ModuleLoader.resolvePaths config.root is used:
filePath = isESM ? require.resolve(path.join(config.root, modulePath))

However this could be a solution (tests pass with this below):

let m try { const p = path.join(this.pjson.oclif.commands as string, ...id.split(':')) const {isESM, module, filePath} = await ModuleLoader.loadWithData(this, p) this._debug(isESM ? '(import)' : '(require)', filePath) m = module } catch (error) { if (!opts.must && error.code === 'MODULE_NOT_FOUND') return throw error }

A further note is that since error.code is being checked in the catch statement that it should be verified that the code is the same for require and dynamic import. My assumption is that may not be the case.

@amphro It turns out in a CommonJS environment that both require and dynamic import return an error code of MODULE_NOT_FOUND.

It is interesting to note that in an ESM environment dynamic import has an error code of ERR_MODULE_NOT_FOUND and require through module.createRequire returns MODULE_NOT_FOUND.

I did one more check from my ESM Oclif CLI and indeed ERR_MODULE_NOT_FOUND is returned for dynamic import internally in @oclif/core, so the error.code check should be changed to:
if (!opts.must && (error.code === 'MODULE_NOT_FOUND' || error.code === 'ERR_MODULE_NOT_FOUND')) return

This is the only location in @oclif/core that checks the error code, but this is something to keep in mind for other Oclif plugins.

Another possible solution since all loading of commands / hooks / help class will be going through ModuleLoader is to add a try / catch to ModuleLoader.load and ModuleLoader.loadWithData and throw a custom Error that is the same for all execution environments.

A reason that this might be a good idea is that error messages displayed to users / developers will be the same for both require / dynamic import in either a CJS or ESM environment and any code in Oclif can use a consistent error.code.

A final note.. If the proposed solution with Module.loadWithData is used in plugin.ts / findCommand then ModuleLoader.importDynamic should be removed.

I have to say I'm certainly shying away from any custom error wrapping of the loading process. MODULE_NOT_FOUND/ ERR_MODULE_NOT_FOUND is one of many errors that can be thrown; IE various errors in the code being loaded itself should be posted.

So, I have updated the PR with what I think is best.

For reference I have implemented my own dual Node / browser module loading code as an ESM Node module that is similar to what is being discussed here (has test cases / test framework):
https://github.com/typhonjs-node-utils/loader-module

amphro · 2021-05-07T05:15:50Z

src/module-loader.ts

+ */
+const _importDynamic = new Function('modulePath', 'return import(modulePath)') // eslint-disable-line no-new-func
+
+export default class ModuleLoader {


I thought your original PR had tests? Is this worth unit testing?

There were no added tests. I'm certainly willing to submit tests as well.

At the bare minimum, can you add a new fixture for a esm and a main test for it here.

amphro · 2021-05-07T05:26:12Z

I've tried this branch on a couple of oclif/core cli's that we have and things look good. I haven't tested on a distributed cli with plugins installed but @oclif/plugin-plugins hasn't been updated yet to use core anyways. I'm good to merge this once I get your responses to my comments.

amphro

I like the changes! I liked the idea of ModuleLoader throwing a custom error for module not found, but I don't really care. Catching both is totally fine. In the case of a custom error, I definitely wouldn't try to wrap every type of error either. You could also put a comment why the two types of errors.

amphro · 2021-05-10T19:14:37Z

After you get the tests in, I'll merge this in.

typhonrt · 2021-05-11T00:52:50Z

I like the changes! I liked the idea of ModuleLoader throwing a custom error for module not found, but I don't really care. Catching both is totally fine. In the case of a custom error, I definitely wouldn't try to wrap every type of error either. You could also put a comment why the two types of errors.

I suppose that is true regarding only handling the two module not found error codes then passing through everything else. I'm thinking a custom CLIError is the way to go. I'll put it in ./src/errors/errors and export it through ./src/errors/index.ts

This is what I'm thinking:

import {CLIError} from './cli'
import {OclifError} from '../../interfaces'

export class ModuleLoadError extends CLIError implements OclifError {
  oclif!: { exit: number }

  code = 'MODULE_NOT_FOUND'

  constructor(message: string) {
    super(`[MODULE_NOT_FOUND] ${message}`, {exit: 1})
    this.name = 'ModuleLoadError'
  }

  render(): string {
    return ''
  }
}

I do think having a custom error makes a lot of sense for Oclif because the runtime is only Node / V8 and it provides a concise and unified error stack leading back to ModuleLoader with one defined error for require or dynamic import when the module is not found. The errors printed out are quite a bit different and verbose in the case of require versus dynamic import and it easily could confuse someone not accustomed to knowing what to expect in advance. The require error lists the stack leading back to ModuleLoader, but the dynamic import error has an internal Node stack that gives no indication of where the error actually occurred.

This is what the error message will be like (paths substituted):

     ModuleLoadError: [MODULE_NOT_FOUND] require failed to load /some/path/to/cli/bad-path.js
      at Function.loadWithData (node_modules/@oclif/core/lib/module-loader.js:64:15)
      at fetch (node_modules/@oclif/core/lib/config/plugin.js:188:62)
      at Plugin.findCommand (node_modules/@oclif/core/lib/config/plugin.js:201:23)
      at Object.load (node_modules/@oclif/core/lib/config/plugin.js:131:54)
      at Config.runCommand (node_modules/@oclif/core/lib/config/config.js:261:29)
      at Object.run (node_modules/@oclif/core/lib/main.js:64:16)

for dynamic import: ModuleLoadError: [MODULE_NOT_FOUND] import() failed to load /some/path/to/cli/bad-path.js

amphro · 2021-05-11T03:47:04Z

Ya, I think that looks good. How come you empty out the render?

dynamic import error has an internal Node stack that gives no indication of where the error actually occurred

Why is that? Wouldn't creating the new CLIError create a new stack trace from the module loader class?

typhonrt · 2021-05-11T04:42:44Z

Ya, I think that looks good. How come you empty out the render?

Just took the initial blueprint from ExitError, so I'll remove it and let CLIError do its thing (render).

dynamic import error has an internal Node stack that gives no indication of where the error actually occurred
Why is that? Wouldn't creating the new CLIError create a new stack trace from the module loader class?

The new ModuleLoadError gives a reasonable stack trace from ModuleLoader as shown above.

I meant the actual stock ESM dynamic import error is not the greatest; below is the normal error:

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/badpath.js' imported from /some/path/to/cli/node_modules/@oclif/core/lib/module-loader.js
    at new NodeError (node:internal/errors:363:5)
    at finalizeResolution (node:internal/modules/esm/resolve:307:11)
    at moduleResolve (node:internal/modules/esm/resolve:742:10)
    at Loader.defaultResolve [as _resolve] (node:internal/modules/esm/resolve:853:11)
    at Loader.resolve (node:internal/modules/esm/loader:89:40)
    at Loader.getModuleJob (node:internal/modules/esm/loader:242:28)
    at Loader.import (node:internal/modules/esm/loader:177:28)
    at importModuleDynamically (node:internal/modules/cjs/loader:1048:29)
    at importModuleDynamicallyWrapper (node:internal/vm/module:437:21)
    at importModuleDynamically (node:vm:387:43) {
  code: 'ERR_MODULE_NOT_FOUND'
}

…ndition

typhonrt · 2021-05-13T03:43:06Z

@amphro This PR should be good to merge now. I added ModuleLoadError to handle the CJS & ESM loader variance with the module not found case and added reasonably thorough unit tests for ModuleLoader in ./test/module-loader.

amphro · 2021-05-13T21:48:29Z

Love the changes. Nice work on this.

integrated ESM loading of commands & hooks

28566d7

salesforce-cla bot added the cla:signed label May 7, 2021

typhonrt mentioned this pull request May 7, 2021

ES Module support for Oclif v2 #130

Closed

amphro reviewed May 7, 2021

View reviewed changes

module-loader / remove importDynamic / update plugin.ts

1ab5f7e

amphro approved these changes May 10, 2021

View reviewed changes

typhonrt added 2 commits May 12, 2021 20:37

Added ModuleLoadError to combine CJS & ESM loader module not found co…

0d0c734

…ndition

Added unit test for ModuleLoader in ./test/module-loader

13f95d7

typhonrt added 2 commits May 13, 2021 11:44

comment clarity & comments in tests

5c8e48a

correct method to test for ModuleLoader.load

f8ba934

amphro merged commit ff47444 into oclif:master May 13, 2021

typhonrt mentioned this pull request May 18, 2021

Strengthened ModuleLoader & unit tests; now supports mixed ESM / CJS plugins #163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate ESM loading of commands & hooks #160

Integrate ESM loading of commands & hooks #160

typhonrt commented May 7, 2021 •

edited

amphro May 7, 2021

typhonrt May 7, 2021

typhonrt May 7, 2021

typhonrt May 7, 2021 •

edited

typhonrt May 8, 2021 •

edited

amphro May 7, 2021

typhonrt May 7, 2021

amphro May 10, 2021

amphro commented May 7, 2021

amphro left a comment

amphro commented May 10, 2021

typhonrt commented May 11, 2021 •

edited

amphro commented May 11, 2021

typhonrt commented May 11, 2021 •

edited

typhonrt commented May 13, 2021

amphro commented May 13, 2021

Integrate ESM loading of commands & hooks #160

Integrate ESM loading of commands & hooks #160

Conversation

typhonrt commented May 7, 2021 • edited

amphro May 7, 2021

Choose a reason for hiding this comment

typhonrt May 7, 2021

Choose a reason for hiding this comment

typhonrt May 7, 2021

Choose a reason for hiding this comment

typhonrt May 7, 2021 • edited

Choose a reason for hiding this comment

typhonrt May 8, 2021 • edited

Choose a reason for hiding this comment

amphro May 7, 2021

Choose a reason for hiding this comment

typhonrt May 7, 2021

Choose a reason for hiding this comment

amphro May 10, 2021

Choose a reason for hiding this comment

amphro commented May 7, 2021

amphro left a comment

Choose a reason for hiding this comment

amphro commented May 10, 2021

typhonrt commented May 11, 2021 • edited

amphro commented May 11, 2021

typhonrt commented May 11, 2021 • edited

typhonrt commented May 13, 2021

amphro commented May 13, 2021

typhonrt commented May 7, 2021 •

edited

typhonrt May 7, 2021 •

edited

typhonrt May 8, 2021 •

edited

typhonrt commented May 11, 2021 •

edited

typhonrt commented May 11, 2021 •

edited