From 888d186ea4dc5918d1ca75dcc5cb0f267221c112 Mon Sep 17 00:00:00 2001
From: Guy Bedford <guybedford@gmail.com>
Date: Sun, 17 Jun 2018 15:56:08 +0200
Subject: [PATCH] beautiful error messages

---
 doc/api/errors.md                             | 11 +++++++++
 lib/internal/errors.js                        | 18 ++++++++++++--
 lib/internal/modules/esm/loader.js            | 24 ++++++++++++++-----
 .../es-module/test-esm-loader-invalid-url.mjs |  6 +++--
 4 files changed, 49 insertions(+), 10 deletions(-)

diff --git a/doc/api/errors.md b/doc/api/errors.md
index c0773caf7c9bbc..487d558ec3f369 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -1204,6 +1204,17 @@ An invalid `options.protocol` was passed.
 Both `breakEvalOnSigint` and `eval` options were set in the REPL config, which
 is not supported.
 
+<a id="ERR_INVALID_RETURN_PROPERTY"></a>
+### ERR_INVALID_RETURN_PROPERTY
+
+Thrown in case a function option does not return an expected property type.
+
+<a id="ERR_INVALID_RETURN_PROPERTY_STRING"></a>
+### ERR_INVALID_RETURN_PROPERTY_STRING
+
+Thrown in case a function option does not return an expected string property
+type.
+
 <a id="ERR_INVALID_RETURN_VALUE"></a>
 ### ERR_INVALID_RETURN_VALUE
 
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 51bf07adf3d91b..47f9096d0a9616 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -681,6 +681,20 @@ E('ERR_INVALID_PROTOCOL',
   TypeError);
 E('ERR_INVALID_REPL_EVAL_CONFIG',
   'Cannot specify both "breakEvalOnSigint" and "eval" for REPL', TypeError);
+E('ERR_INVALID_RETURN_PROPERTY', (input, name, prop, value) => {
+  let type;
+  if (value && value.constructor && value.constructor.name) {
+    type = `instance of ${value.constructor.name}`;
+  } else {
+    type = `type ${typeof value}`;
+  }
+  return `Expected ${input} to be returned for the ${prop} from the ` +
+         `"${name}" function but got ${type}.`;
+}, TypeError);
+E('ERR_INVALID_RETURN_PROPERTY_STRING', (input, name, prop, value) => {
+  return `Expected a valid ${input} to be returned for the ${prop} from the ` +
+         `"${name}" function but got ${value}.`;
+}, TypeError);
 E('ERR_INVALID_RETURN_VALUE', (input, name, value) => {
   let type;
   if (value && value.constructor && value.constructor.name) {
@@ -688,8 +702,8 @@ E('ERR_INVALID_RETURN_VALUE', (input, name, value) => {
   } else {
     type = `type ${typeof value}`;
   }
-  return `Expected ${input} to be returned from the "${name}"` +
-          ` function but got ${type}.`;
+  return `Expected ${input} to be returned from the ` +
+         `"${name}" function but got ${type}.`;
 }, TypeError);
 E('ERR_INVALID_SYNC_FORK_INPUT',
   'Asynchronous forks do not support Buffer, Uint8Array or string input: %s',
diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
index bcc15d28370e79..c53d16d3cf203d 100644
--- a/lib/internal/modules/esm/loader.js
+++ b/lib/internal/modules/esm/loader.js
@@ -2,7 +2,8 @@
 
 const {
   ERR_INVALID_ARG_TYPE,
-  ERR_INVALID_PROTOCOL,
+  ERR_INVALID_RETURN_PROPERTY,
+  ERR_INVALID_RETURN_PROPERTY_STRING,
   ERR_MISSING_DYNAMIC_INTSTANTIATE_HOOK,
   ERR_UNKNOWN_MODULE_FORMAT
 } = require('internal/errors').codes;
@@ -57,21 +58,32 @@ class Loader {
       await this._resolve(specifier, parentURL, defaultResolve);
 
     if (typeof url !== 'string')
-      throw new ERR_INVALID_ARG_TYPE('url', 'string', url);
+      throw new ERR_INVALID_RETURN_PROPERTY(
+        'string', 'loader resolve', 'url', url
+      );
 
     if (typeof format !== 'string')
-      throw new ERR_INVALID_ARG_TYPE('format', 'string', format);
+      throw new ERR_INVALID_RETURN_PROPERTY_STRING(
+        'string', 'loader resolve', 'format', format
+      );
 
     if (format === 'builtin')
       return { url: `node:${url}`, format };
 
     if (this._resolve !== defaultResolve) {
-      // throws ERR_INVALID_URL for resolve if not valid
-      new URL(url);
+      try {
+        new URL(url);
+      } catch (e) {
+        throw new ERR_INVALID_RETURN_PROPERTY_STRING(
+          'url', 'loader resolve', 'url', url
+        );
+      }
     }
 
     if (format !== 'dynamic' && !url.startsWith('file:'))
-      throw new ERR_INVALID_PROTOCOL(url, 'file:');
+      throw new ERR_INVALID_RETURN_PROPERTY_STRING(
+        'file: url', 'loader resolve', 'url', url
+      );
 
     return { url, format };
   }
diff --git a/test/es-module/test-esm-loader-invalid-url.mjs b/test/es-module/test-esm-loader-invalid-url.mjs
index d1ef29cf034ff2..440f8d8deff201 100644
--- a/test/es-module/test-esm-loader-invalid-url.mjs
+++ b/test/es-module/test-esm-loader-invalid-url.mjs
@@ -1,10 +1,12 @@
 // Flags: --experimental-modules --loader ./test/fixtures/es-module-loaders/loader-invalid-url.mjs
 /* eslint-disable node-core/required-modules */
 import assert from 'assert';
+import common from '../common';
 
 import('../fixtures/es-modules/test-esm-ok.mjs')
 .then(() => {
   assert.fail();
 }, (err) => {
-  assert.strictEqual(err.code, 'ERR_INVALID_URL');
-});
+  assert.strictEqual(err.code, 'ERR_INVALID_RETURN_PROPERTY_STRING');
+})
+.then(common.mustCall());