UnhandledPromiseRejection when plugins callback with failure about metalsmith

[webketje]

@emmercm closing since v2.6.2 just got published

[webketje]

So I did a quick test locally and was unable to reproduce with your exact example. However, it does occur if the build/process callback has a if (err) throw err and the parameter passed to the done function is not an instance of Error (so there is your "easy" fix). The same happens when the plugin does Promise.reject('test'), or when metalsmith.process is used directly.

The unhandled rejection is actually thrown IN the build/process callback. It enters the error callback at https://github.com/metalsmith/metalsmith/blob/v2.6.1/lib/index.js#L565 and in fact when I simply change this to:

@@ -563,7 +563,10 @@ Metalsmith.prototype.process = function (callback) {

     /* block required for Metalsmith 2.x callback-flow compat */
     if (callback) {
-      result.then((files) => callback(null, files), callback)
+      result.then((files) => callback(null, files), err => {
+        if (!(err instanceof Error)) err = new Error(err.toString())
+        callback(err)
+      })
     } else {
       return result

it works. I'm not too keen on adding this code because it looks like a band-aid for an underlying issue that I haven't gotten to the bottom of yet.

[emmercm]

Here's a complete example from a completely fresh directory:

$ cd $(mktemp -d)


$ volta install [email protected]
success: installed and set [email protected] (with [email protected]) as default


$ node -v                     
v16.20.2


$ npm init -y
Wrote to /private/var/folders/v5/94q5n_25731dk5hrlwrb9p2r0000gn/T/tmp.Lt5esvG8MU/package.json:

{
  "name": "tmp.lt5esvg8mu",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC"
}


$ npm install [email protected]     

added 36 packages, and audited 37 packages in 870ms

2 packages are looking for funding
  run `npm fund` for details

found 0 vulnerabilities


$ cat >index.js <<EOL
const Metalsmith = require('metalsmith');
const fs = require("fs");

if (!fs.existsSync('src')) {
  fs.mkdirSync('src');
}

Metalsmith('.')
  .use((files, metalsmith, done) => {
    done('error');
  })
  .build((err) => {});
EOL


$ node index.js
node:internal/process/promises:279
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "error".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

The example doesn't do any type checking (as I haven't converted my plugins to TypeScript yet).

This error happens on both a 2019 Intel MacBook Pro and an M2 MacBook Air, so I don't think it's an Apple Silicon issue.

---

Metalsmith has always supported strings as the error message, but changing it to a proper Error might solve the issue?

const Metalsmith = require('metalsmith');
const fs = require("fs");

if (!fs.existsSync('src')) {
  fs.mkdirSync('src');
}

Metalsmith('.')
  .use((files, metalsmith, done) => {
    done(new Error('error'));
  })
  .build((err) => {});

$ node index.js
/private/var/folders/v5/94q5n_25731dk5hrlwrb9p2r0000gn/T/tmp.Lt5esvG8MU/index.js:10
    done(new Error('error'));
         ^

Error: error
    at Ware.<anonymous> (/private/var/folders/v5/94q5n_25731dk5hrlwrb9p2r0000gn/T/tmp.Lt5esvG8MU/index.js:10:10)
    at Ware.<anonymous> (/private/var/folders/v5/94q5n_25731dk5hrlwrb9p2r0000gn/T/tmp.Lt5esvG8MU/node_modules/wrap-fn/index.js:45:19)
    at next (/private/var/folders/v5/94q5n_25731dk5hrlwrb9p2r0000gn/T/tmp.Lt5esvG8MU/node_modules/ware/lib/index.js:85:20)
    at Ware.run (/private/var/folders/v5/94q5n_25731dk5hrlwrb9p2r0000gn/T/tmp.Lt5esvG8MU/node_modules/ware/lib/index.js:88:3)
    at /private/var/folders/v5/94q5n_25731dk5hrlwrb9p2r0000gn/T/tmp.Lt5esvG8MU/node_modules/metalsmith/lib/index.js:601:5
    at new Promise (<anonymous>)
    at Metalsmith.run (/private/var/folders/v5/94q5n_25731dk5hrlwrb9p2r0000gn/T/tmp.Lt5esvG8MU/node_modules/metalsmith/lib/index.js:600:18)
    at /private/var/folders/v5/94q5n_25731dk5hrlwrb9p2r0000gn/T/tmp.Lt5esvG8MU/node_modules/metalsmith/lib/index.js:560:42

[emmercm]

I think it's this exact change:

9d40674#diff-92bbac9a308cd5fcf9db165841f2d90ce981baddcb2b1e26cfff170929af3bd1R541

The result of this.run() can be a rejected promise:

metalsmith/lib/index.js

Line 592 in 9d40674

return result

which requires explicit catching as part of:

result.then((files) => this.run(files, this.plugins))

as in:

result.then((files) => this.run(files, this.plugins)).catch((err) => { /* */ });

[webketje]

Sorry, I was testing with the latest master locally instead of the tag and validated that this as of yet unreleased commit actually fixes it: 878fb79.

I'll release it under 2.6.2 ASAP

[emmercm]

Awesome, thank you, @webketje!

I spent a few days converting my plugins to TypeScript, but I had to copy the v2.6.1 type definitions over because the upgrade is held back: https://github.com/emmercm/metalsmith-plugins/blob/fa0cd9968da9e8644ef028484a9aeb5a8025326e/%40types/metalsmith.d.ts

[webketje]

@emmercm I don't quite understand why you had to copy the type definitions? What does it have to do with the current issue?

[emmercm]

Because I currently can't upgrade to v2.6 because of this issue, but I wanted to convert my plugins to TypeScript. I should be able to get rid of the type definitions once v2.6.2 is released.