Update DB error handling to reject with Error objects

[plikarish]

Promise rejections generate PromiseRejectionEvents (PREs). PREs allow specifying a reason.
Rejecting with an Error object improves debugging because the Error will contain an Error message and stack trace. DOMExceptions and DOMErrors do not extend Error but can be passed as the underlying cause in the Error to retain the original IDB request or transaction error details.

[jakearchibald]

DOMExceptions and DOMErrors do not extend Error

(new DOMException) instanceof Error === true, but that isn't true for DOMError. Can you give me an example where interaction with the library raises a DOMError so I can write tests for this?

If the fix only 'rewrote' DOMErrors rather than DOMExceptions, would that be ok?

[plikarish]

If the fix only 'rewrote' DOMErrors rather than DOMExceptions, would that be ok?

TY Jake, totally agree that all browsers return true when comparing DOMExceptions with Error. I use the closure compiler and I can't convinced it the above is true: I have a conformance check run by the compiler that asserts that reject(...) calls throw objects of type Error and it fails if I only handle the DOMError case.

I think this is b/c idl does lists DOMExceptions as a "2nd type of exception" alongside JSError objects. DOMException's internal slot is defined as %Error.prototype% and should get the same "special powers" as Errors so it makes sense they should extend Error, just not sure that's true everywhere.

wdyt of:

reject(tx.error instanceof Error ?? tx.error : new Error('IDBError', {cause: tx!.error}));

This should effectively be a no-op (except for DOMError cases) as well as satisfying the compiler.

Can you give me an example where interaction with the library raises a DOMError so I can write tests for this?

This should only happen on very old browsers--there's no constructor for DOMError so I also haven't been able to write tests for this case unless we literally stub out the class in the test file... To support these older browsers the return type from the IDB APIs has remained DOMException|DOMError so that's why I was looking to handle both.

[jakearchibald]

there's no constructor for DOMError

new DOMError('yo') works for me.

But I'm less interested in a test that fakes this, and more interested in the real world case where this has been an issue.

I'm assuming that you've hit a case where this library has rejected a promise with a DOMError. I'm trying to figure out how that happened. Was the error thrown by IndexedDB? I can't see anywhere in the spec where that happens, but maybe an implementation is doing something non-standard?

[jakearchibald]

Or, is this not actually a runtime issue, and just an issue with closure compiler's type system? If that's the case, I'd rather find a way that doesn't increase the size of this library, and doesn't change types of errors folks might be expecting.

I'm not that familiar with closure compiler these days. What's making it think that DOMError may be returned?

[jakearchibald]

Would reject(request.error! satisfies Error) do the trick? I have no idea how much closure compiler leans on TS

[plikarish]

Would reject(request.error! satisfies Error) do the trick? I have no idea how much closure compiler leans on TS

I tested this and it solves one of my issues (.error field is nullable, so the compiler's smart enough to know it's non-null in this case) but I don't think it narrows the type based on the satisfies :/ (thanks for suggesting btw, I didn't think of that!)

Or, is this not actually a runtime issue, and just an issue with closure compiler's type system?

I strongly suspect this is the case. The DOMError piece should only occur on very old browsers that see minimal adoption at this point.

Specifically the versions between the IDBRequest and DOMException value instead of Error rows:
Chrome: 24-47
Edge: 12-17
Firefox: 16-57
Opera: 15-34
Safari: ?

Source

I'd rather find a way that doesn't increase the size of this library, and doesn't change types of errors folks might be expecting.

That's very understandable! Some options, lmkwyt:

1. I could add a comment instructing the closure compiler to consider .error a non-nullable Error with the following annotations: reject(/** @type {!Error} */ (request.error)); before returning request.error or tx.error. No runtime impact but I acknowledge the comment could look odd to library users, happy to add more context to explain it in another comment if useful :)
2. If you're open to checking instanceof Error (just repeating comment above) before returning the Error this should functionally be a no-op since it only impacts the DOMError case and the versions above are pretty old.
3. Proceed with request.error! satisfies Error -- this doesn't fully solve my problem but resolving nullability is also directionally helpful to me.
4. Reject the PR--I appreciate you iterating with me but I'm sure it goes without saying I defer to you if there's not a way to land this :)

[plikarish]

Hey Jake, not trying to rush you, just a quick update that this may actually help diagnose runtime issues. One of the developers I work with suspects we may not be getting useful stack traces in some IDB-related cases. Their report on a PR on their own repo:

• We are still seeing many apparently IDB related errors without stack traces. This change is an attempt to find out where they may be from.
• IDB provides DOMException at the request and transaction level. The fact that we are getting errors reported without a stack trace might be a problem in the idb_ts lib we are using, which will hopefully become clearer after this change.

[masonicboom]

@plikarish this might be your issue: #338 (comment)