mochajs / mocha

☕️ simple, flexible, fun javascript test framework for node.js & the browser
https://mochajs.org
MIT License
22.59k stars 3.01k forks source link

🚀 Feature: Expose `PluginLoader` or `handleRequires` to have other frameworks provide support for the `require` option #4961

Open christian-bromann opened 1 year ago

christian-bromann commented 1 year ago

Is your feature request related to a problem or a nice-to-have?? Please describe. For frameworks such as WebdriverIO that integrate Mocha, it would be great to support the require option.

Describe the solution you'd like For that to happen, Mocha should export the PluginLoader or have the handleRequires exposed from the package.

Describe alternatives you've considered None.

Additional context Issue filed in the WebdriverIO project: https://github.com/webdriverio/webdriverio/issues/9490

juergba commented 1 year ago

@christian-bromann Your above mentioned webdriverio issue has already been fixed and closed. So would do you need exactly?

Mocha's --require (not identical to Node's --require) is a public option and supports both sync and async modules. It can be used to pre-load any module or to define some mochaHooks.

christian-bromann commented 1 year ago

@juergba thanks for getting back. The PR currently imports handleRequires which is not ideal because the location of it can change with every release.

// @ts-expect-error not exposed from package yet, see https://github.com/mochajs/mocha/issues/4961
import { handleRequires } from 'mocha/lib/cli/run-helpers.js'

I think to make the Mocha integration work it would be great to either export PluginLoader or the handleRequires method. Would that be possible? Happy to file a PR.

juergba commented 1 year ago

@christian-bromann yes, it's possible. I'm unsure which way to go wether PluginLoader or handleRequires.

PluginLoader was intended to eventually expose it later to the public, but the changes for your purpose seem to be far more complex.

handleRequires is the method which lays behind the --require option. So it would be a small step to expose it. Can we expose a method of a private module?

Evtl. you can explain your reasons for choosing the latter way. Then your PR would be welcome, thank you.

JoshuaKGoldberg commented 9 months ago

@christian-bromann I think this is still waiting on you: can you explain why you chose handleRequires over PluginLoader? Does PluginLoader not provide enough API surface for you?

We the new maintainers are still ramping up (#5027) and this is a relatively niche use case (API request for large API user), so it might be a while before we have the time to deeply understand this on our own. Any help you can give us & context on why you implemented things the way you did would be very helpful. 🙂 Thanks!

christian-bromann commented 9 months ago

Hey @JoshuaKGoldberg , I could use the PluginLoader but would need to re-implement all the other logic within handleRequires. All I need is a reliable way to copy what the Mocha CLI is doing so I can easily integrate it in WebdriverIO. For now, I am importing the helper function from Mocha directly:

// @ts-expect-error not exposed from package yet, see https://github.com/mochajs/mocha/issues/4961
import { handleRequires } from 'mocha/lib/cli/run-helpers.js'

which works but is not ideal.

JoshuaKGoldberg commented 8 months ago

Got it, thanks. I took a read through the code and it looks pretty reasonable what wdio is doing: using Mocha's handleRequires to mimic what the mocha --require does on the CLI.

handleRequires is actually a pretty small wrapper around the PluginLoader class. Removing comments and logs from its current implementation roughly leaves:

async function handleRequires(requires = [], { ignoredPlugins = [] } = {}) {
  const pluginLoader = PluginLoader.create({ ignore: ignoredPlugins });

  for await (const mod of requires) {
    let modpath = mod;
    if (fs.existsSync(mod) || fs.existsSync(`${mod}.js`)) {
      modpath = path.resolve(mod);
    }

    const requiredModule = await requireOrImport(modpath);
    if (requiredModule && typeof requiredModule === "object") {
      pluginLoader.load(requiredModule);
    }
  }

  return await pluginLoader.finalize();
}

Per https://github.com/mochajs/mocha/issues/4961#issuecomment-1436031263:

PluginLoader was intended to eventually expose it later to the public

I think it'd make most sense to provide PluginLoader as a public export. That way API integrators such as yourself can work with its logic as they need without being tied to the fs calls and requireOrImport implementation from Mocha.

Note also that #1457 discusses a separate concept of a "Plugin API", which is a kind of unfortunate naming conflict. One of the two will need to be renamed. No thoughts yet on which.

cc @mochajs/maintenance-crew as this would be a big new API piece - thoughts?