loaders: super confusing "expected string but got instance of string"

[isaacs]

Version

21.0.0-pre, 20.6.0, 18.17.1

Platform

Darwin moxy.lan 22.6.0 Darwin Kernel Version 22.6.0: Wed Jul 5 22:22:05 PDT 2023; root:xnu-8796.141.3~6/RELEASE_ARM64_T6000 arm64

Subsystem

loaders, modules, errors

What steps will reproduce the bug?

// oops.mjs
export const resolve = (url, context, nextResolve) => {
  // bug, yes, but the error is SUPER confusing
  if (url === 'events') {
    return {
      url,
      context,
      shortCircuit: true,
    }
  }
  return nextResolve(url, context)
}

// program.mjs
import EventEmitter from 'events' // boom

$ ./node --loader=./oops.mjs program.mjs
(node:42540) ExperimentalWarning: Custom ESM Loaders is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

node:internal/process/esm_loader:48
      internalBinding('errors').triggerUncaughtException(
                                ^
TypeError [ERR_INVALID_RETURN_PROPERTY_VALUE]: Expected a URL string to be returned for the "url" from the "./oops.mjs 'resolve'" function but got instance of String.
    at new NodeError (node:internal/errors:405:5)
    at Hooks.resolve (node:internal/modules/esm/hooks:309:15)
    at async MessagePort.handleMessage (node:internal/modules/esm/worker:176:18) {
  code: 'ERR_INVALID_RETURN_PROPERTY_VALUE'
}

Node.js v21.0.0-pre

How often does it reproduce? Is there a required condition?

If the resolve() or load() function in a loader accidentally returns a shortCircuit response for a node builtin, or any other string which cannot be parsed as a URL.

What is the expected behavior? Why is that the expected behavior?

The error should indicate that the expected value should be a URL-parseable string, but instead got a string that could not be parsed.

What do you see instead?

The error is very confusing, indicating that a string was expected, but instead an instance of string was returned. In fact, 'events' instanceof String is clearly false, so this is not really an "instance of String".

Additional information

Maybe in cases like these, the string itself should be included on the Error object or in the message. It would've been immediately clear to me that 'events', while being a string, is not a valid URL. However, the wording of the error message had me thinking I'd mistakenly done new String() somewhere instead of casting to a string primitive.

At the very least, the "instance of" wording should only be included if typeof value === 'object' && value !== null, because primitives are not instanceof anything.

[aduh95]

The error is very confusing, indicating that a string was expected, but instead an instance of string was returned.

I don't know if you miss it, or if you are just not talking about it, but the error message says it expected a URL string.
The rest of the complain is fair IMO, the instance of String is weird indeed.

[isaacs]

I don't know if you miss it, or if you are just not talking about it, but the error message says it expected a URL string. The rest of the complain is fair IMO, the instance of String is weird indeed.

Well, I did see that, but I was thinking that my code was only ever returning a stringified URL object, and forgot that the resolveImport() module will return builtins as their unmodified strings, so I had discounted the possibility that the string wasn't a valid URL. Printing the string in the error is 💯, would have solved it right away.