Removal of resolve module fallback in v8.0.0 can cause resolution issues in (webpack) bundled code

[wpessers]

Context

Recently a fallback on the resolve module was removed in: 67076ff#diff-e727e4bdf3657fd1d798edcd6b099d6e092f8573cba266154583a746bba0f346L73-L82 This was part of the 8.0.0 release.

This fallback seems not to have been intended to support resolving external modules from inside bundled code, but as a side effect it actually did. When bundling code with Webpack, at runtime Webpack provides its own loader instead of the node.js native require.resolve. In short this means that when using require.resolve inside code bundled with Webpack, it can only resolve modules that are contained in the bundle. So external modules can't possibly be resolved this way.

The above linked commit basically removes a fallback to the resolve module for when (require.resolve && require.resolve.paths) doesn't evaluate to truthy. Webpack doesn't have require.resolve.paths on its "stubbed" require.resolve since it isn't useful / needed for the way that module resolution works in the Webpack runtime. So the check would fail since the require.resolve.paths is undefined (and thus falsy). This lead to the resolve module being used instead, with which you were able to actually resolve those external modules.

The reason I found this was because we did some dependency upgrades in the https://github.com/open-telemetry/opentelemetry-lambda layer for nodejs. Upgrading instrumentation libraries that use RITM indirectly through @opentelemetry/instrumentation. The latter upgraded the RITM version from 7.1.1 to 8.0.0. And now since the code for the node.js layer is bundled with Webpack, that means RITM inside our bundle will be actually using Webpack's stubbed require.resolve. This results in breaking instrumentation that needs to hook any non-core modules through RITM. Because those modules need to be external (not included in the bundle) for instrumentation libraries to be able to detect them being loaded.

Solutions

We have temporarily fixed this in the lambda layer by externalizing RITM from the webpack bundle (open-telemetry/opentelemetry-lambda#2037), but would like to see if any alternative solutions are possible.

I suppose the removal of the resolve module was a very deliberate choice to reduce footprint / increase performance?

One solution I could think of would be to have some check for when ritm is running inside a webpack bundle, pseudo-code but could look something like this:

function resolve (moduleName, basedir) {
    let _resolve = function (moduleName, basedir) {
        return require.resolve(moduleName, { paths: [basedir] })
    }
    if (webpackDetected) {
        _resolve = function (moduleName, basedir) {
            return __non_webpack_require__.resolve(moduleName, { paths: [basedir] })
        }
    }
}

[serkan-ozal]

It seems a common issue when RITM is bundled and require calls are replaced by bundler specific loader (which is __webpack_require__ in webpack case) or removed at all (not sure how other bundlers behave in this case).

Specifically, to handle this case in Webpack, we may use something like below to get the Node runtimes native require or it should be documented that RITM users who bundle it should define it as external dependency:

function getNodeRequire() {
  // 1) Webpack-specific escape hatch:
  //    If Webpack is bundling this file (target: node), it exposes
  //    `__non_webpack_require__` as the original Node.js require.
  if (typeof __non_webpack_require__ === 'function') {
    return __non_webpack_require__;
  }

  // 2) If the bundler did NOT replace require (esbuild, Rollup,
  //    or any bundler running in node platform mode), then the local
  //    `require` is still the native Node.js require.
  if (typeof require === 'function') {
    return require;
  }

  // 3) Fallback for when the bundler transformed/removed the top-level require:
  //    Using `eval('require')` avoids static analysis and usually bypasses
  //    bundler transformations, giving us the real Node.js require at runtime.
  try {
    // eslint-disable-next-line no-eval
    return eval('require');
  } catch (_) {}

  // 4) If nothing worked, return null. (This should not happen in Node.)
  return null;
}

@trentm WDYT?

[timfish]

I suppose the removal of the resolve module was a very deliberate choice to reduce footprint / increase performance?

resolve was only technically used for Node <8.10.0. It significantly cuts the number of dependencies for anyone using a Node version released in the last 7 years.

For RITM to work when code is bundled you need to ensure that the modules getting patched are external. At that point it doesn't seem a big issue to also add require-in-the-middle to this list?

[wpessers]

resolve was only technically used for Node <8.10.0. It significantly cuts the number of dependencies for anyone using a Node version released in the last 7 years.

Yes, makes sense to me.

Also I'm not entirely opposed to keeping require-in-the-middle external on our end. Just wanted to check here since while indeed technically resolve was used only for Node <8.10.0, we've just found out that that wasn't entirely the case due to the described webpack behaviour. So I believed it made sense to check if there is any desire to cover that in RITM, because it was accidentally covered before.

If maintainers here don't deem this to be a big concern, fine by me as well! Part of the intent of opening the issue was to document it here, so that if other people run into this they can at least find this information and now what to do.

[trentm]

I wouldn't be opposed to adding some handling in RITM to check that it actually has the Node.js require.resolve() before calling it -- something like the proposed snippets above.

@timfish Not a big issue to also add require-in-the-middle to the "externals" list, but perhaps the user shouldn't have to know about this.

For the OpenTelemetry use-case, for example, RITM is a transitive dep that the user might not be aware of. I guess the current OTel JS instrumentation bundler "Limitations" notes here are insufficient as of RITM@8, because the user would have to made both their used modules and RITM external. OTel JS only has very limited bundler tests (open-telemetry/opentelemetry-js#5919) that don't catch this, so things could definitely be improved on that side.

---

It is kind of annoying to have to consider Webpack-specific workarounds (e.g. checking __non_webpack_require__) inside unrelated libraries. I found it interesting from @serkan-ozal's comment that other bundlers do not replace Node's require. So perhaps this "work around bundlers" added code doesn't open the door to a large block to handle every bundler under the sun.

Slightly related, I guess the current try { res = require.resolve(...) } catch (e) { ... } block will always be throwing for SEA applications? (c.f. #104)

[trentm]

Regarding the eval(...) usage: open-telemetry/opentelemetry-js#4987 and open-telemetry/opentelemetry-js#6179 suggest that adding any code using eval(...) may cause problems for other bundlers. :)