false positive: use of require.resolve causes unexpected warning (v0.7.4)

[rsms]

I appreciate the helpful warning messages esbuild emits when it detects that I've done something janky. However it seems that the sophistication of detecting use or require could use some improvement, in particular with the require.resolve function.

Repro: main.js

console.log(require.resolve("foo"))

$ esbuild --version
0.7.4
$ esbuild --bundle main.js
main.js:1:12: warning: Indirect calls to "require" will not be bundled
console.log(require.resolve("foo"))
            ~~~~~~~

For a program like Estrella it is sometimes useful to poke around for node modules without using their code, for example to read package.json files.

[rtsao]

I think most bundlers will statically evaluate require.resolve into an arbitrary string (usually some project-relative path). So if foo is not an external package, I think a warning is actually valid in the case, although the message could be improved.

Perhaps it should be changed to something like:

warning: Calls to require.resolve will not be statically evaluated

That said, perhaps a better solution would be to just implement static evaluation of require.resolve when invoked with plain string literals.

[rsms]

Interesting. I'm surprised to learn that!

I'd argue that from an idealistic standpoint, evaluating require.resolve at compile time is a mistake. It's clearly a runtime function in the world of Nodejs and its result is non-deterministic (depends on the host's state at the time of calling the function.)

Personally I'd like to see only deterministic function results evaluated & inlined by a compiler. For example Math.max(1,2,3) => 3. For other data that a project may want to embed into its code, rather than compute at runtime, I think it's better to use code generation and either use imports or esbuild's define functionality to include it in the output JavaScript file(s).

[rtsao]

I think this behavior originated in browserify and is also present in webpack. Because require doesn't exist in the browser, require.resolve needs to be replaced with something that won't throw an exception.

But even for Node.js bundles, it is generally desirable to replace require.resolve so that your bundle (which in theory includes all your dependencies) is effectively a standalone binary. Leaving runtime require.resolve calls in your bundle would mean your bundle would break if it were to be executed without node_modules installed, which sort of defeats the point of bundling in the first place.

If you want require.resolve("xyz") to happen at runtime, I think it would make sense that you would need to flag xyz as an external dependency.

At least for webpack, for runtime require, you must either declare the dependency as external or use the __non_webpack_require__ escape hatch rather than require directly.

[evanw]

So esbuild doesn't really have a robust solution for bundling projects with dynamic import paths. Some other bundlers such as Webpack have a complex virtual file system that attempts to include files in the bundle that you may be trying to import and then resolve the paths to the bundled files at run time. In that context statically evaluating require.resolve could make sense. This is complexity I'm not planning to include in esbuild itself.

Because esbuild doesn't handle these cases, I think these warnings about unusual uses of require are useful and should stay. The reason is that if the warnings aren't there, the alternative is that things fail silently in ways that only show up at run time, and potentially only when certain code paths are hit. And when they do show up, it may not be obvious why it's happening, especially if the build appears to succeed without any issues.

If the warnings are an aesthetic problem, they can currently either be disabled completely via --log-level=error or individually by filtering the warning list returned by the API before printing them or by moving the use of require into a try block.

If you want require.resolve("xyz") to happen at runtime, I think it would make sense that you would need to flag xyz as an external dependency.

This is an interesting proposal. I think that sounds reasonable to me. It's consistent with how require("xyz") is handled.

[rsms]

I tried flagging the files required and resolved as external but it seems that only "named" imports (as in names of node_modules) are supported.

Here's my scenario:

• I'm building a node program which consists of two products: program.js, debug.js
• The entry point of the program is program.js
• It loads debug.js only when some edge-case stuff happens (to reduce startup cost as its a CLI program)
• Another thing it does it to report the version of esbuild when an error occurs, which is useful for people when they submit bug reports.

Snippet: product file layout

package.json
dist/estrella.js  <-- CLI program
dist/debug.js     <-- 40kB of code loaded as needed

Snippet: Loading additional code just-in-time

Error.prepareStackTrace = (error, stack) => {
  const debug = require(Path.join(__dirname, "debug.js"))
  return debug.prepareStackTrace(error, stack)
}

Snippet: Getting the esbuild version

function getEsbuildVersion() {
  try {
    return JSON.parse(resolveModulePackageFile("esbuild")).version
  } catch (_) {
    return "(unknown)"
  }
}

function resolveModulePackageFile(moduleSpec) {
  const mainfile = require.resolve(moduleSpec)
  let dir = Path.dirname(Path.resolve(mainfile))
  let lastdir = Path.sep
  while (dir != lastdir) {
    let pfile = Path.join(dir, "package.json")
    if (fs.existsSync(pfile)) {
      return pfile
    }
    dir = Path.dirname(dir)
  }
  throw new Error(`package.json not found for module ${moduleSpec}`)
}

An expensive work-around would be to make each product file its own separate node module, flag those as external and import them with e.g. require("estrella-debug"). That would make the project setup too complex IMHO.

For now I've found a good work-around that avoids the warning:

const runtimeRequire = eval("require")
// replace all uses of require with runtimeRequire

Thanks Evan!

[charlesritchea]

@rtsao So what are we supposed to do when require.resolveing a relative file like a web worker that isn't in node_modules? How do I mark that as external?