levelup constructor without callback causes unhandled rejection

[bjacokes]

Using v4.0.1 of levelup and leveldown. The following command causes an unhandled rejection:

levelup(leveldown('/path/to/nonexistent/file'))

UnhandledPromiseRejectionWarning: OpenError: IO error: /path/to/nonexistent/file: No such file or directory
....
    at (node:69500) UnhandledPromiseRejectionWarning: Unhandled promise rejection. 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(). (rejection id: 1)

This happens because the final line in the LevelUP constructor, this.open(callback), is a floating promise.

[vweevers]

Aye. The unhandled rejection is somewhat consistent with the previous behavior of emitting an error event, but the difference is you can't catch it now. Perhaps we should change this line:

levelup/lib/levelup.js

Line 57 in c56f863

this.open(callback)

To:

this.open(callback || function (err {
  if (err) self.emit('error', err)
})

[vweevers]

A PR is welcome!

[bjacokes]

Your proposed change would work, but it seems (maybe?) inconsistent with this line in the docs: "Breaking change: previously, if you did not pass a callback to an async function and there was an error, levelup would emit an error event instead. This is no longer true." I also don't see any other places in the code where the error event is emitted.

I'm happy to open a PR for the code portion, as long as error is still an event that you're OK emitting, and as long as you feel that no doc changes are needed.

(FWIW, I'm not blocked by this in my own project since I switched to using the callback form of the constructor.)

[vweevers]

"Breaking change: previously, if you did not pass a callback to an async function and there was an error, levelup would emit an error event instead. This is no longer true."

That line applies to all methods in general. The open-callback was/is the only special case.

We can document the error event here. E.g. "If you don't provide a callback, any read & write operations are simply queued internally until the store is fully opened, unless it fails to open, in which case an error event will be emitted."

as long as error is still an event that you're OK emitting,

It's OK. The error event is a common pattern in Node.js as well as levelup historically. And it's preferable to an unhandled promise rejection, which is left hanging without crashing the process (though that behavior is gonna change in Node.js I believe).

[jobe451]

I am writing currently a program, where it can happen by design, that database is already in use by a different process. In this case I would like to terminate the program gracefully. It seems that this is not possible at the moment.
It seems I have to work around now this issue in some other way with some additional effort. An error that would allow intercepting this error would be very welcomed.

[vweevers]

A PR is welcome.