Ideally, we would only attach our own Promise if errorFn is defined, but that would require users to register an error function before any other API call. I think that we could re-throw the original exception here though if the error callback is not defined. This can be a simple change if you change the default error function to just throw.

I don't fully follow along here. If we change the default function to throw, that would cause an UnhandledPromiseRejection to surface again. Isn't the point of attaching a .catch() to the existing promise to prevent the UnhandledPromiseRejection from being thrown in the first place?

Something like:

const defaultCatch = (e) => throw e;

set() {
	this.op().catch(() => defaultCatch())
}

onError(errorFn) {
  defaultCatch = errorFn;
}

This should avoid the "uncaught Promise rejection" handling only when onError is called.

typo slipped in here

I don't think I see how it's possible for this to be the case after awaiting the flush, above, because it's not possible to add retries after this.closeCalled = true, right?

If it is expected, does this need to be in a loop?

I now see that verifyNotClosed only throws if fully closed when called from retry. In which case, I think my second question still stands. Do we know that the await this._flush() won't cause more retries? Do we either need to do this in a loop or at least await another flush after this.closeDeferred.promise resolves?

I think we can use Promise chaining here as proposed in the diff above. Instead of requiring void as the return type of onWriteError, we can require the user to call retry and to pass the result of retry to use via the error handler. The signature would then be:

onWriteError((err:BulkWriterError) => Promise<WriteResult>)

Once we do that, we can chain the result of this error callback and block on the retry attempt in the flush call.

If we don't do this, we have to somehow ensure that all error callbacks have fired once we process the results. This would be a bit tricky and may not port to Java or other languages:

error.reject(...);
setImmediate(() => {
  // This only works in Node, but due to the way that ticks are processed in the VM, we can use a `setImmediate`
  // to ensure that our code only runs once all Promise chains are processed.
});

That would be elegant, I think. The ultimate result of the original write would become the result of the retry. It would just be important to document well so that callers don't forget to return the result of retry() from their error handler or the retry promise would end up unhandled and the original write would resolve with undefined.

https://gist.github.com/schmidt-sebastian/ebb63b96c048f3c43187edac7523f9c4

The idea here is that you can use GrpcStatus in the typings file and the Gax status in code. They are interchangeable.

Please address.

I think this is a necessary evil unfortunately.

Do these show up in JSDoc? Should we add comments?

We may want to align this with the status codes in BulkWriterBatch and use an enum (either a new enum or a specific enum for BulkWriter (open/closing/closed)).

By default, you probably want this error function to throw. You still want "Unhandled Promise Rejections" when there is no error handler and no other callback attached.

I think you want to return the Promise that has the error handler attached. That way, the default error handler can re-throw and any user specified error handler can prevent Unhandled Promise Rejections.

Random thought: Should the already build-in retry use the new Public APIs? This would simplify some of the code a bit (https://gist.github.com/schmidt-sebastian/302c19baa2eda89b23435da41afb077f) and it would allow us to use consistent backoff for all retries.

I think we can use Promise chaining here as proposed in the diff above. Instead of requiring void as the return type of onWriteError, we can require the user to call retry and to pass the result of retry to use via the error handler. The signature would then be:

onWriteError((err:BulkWriterError) => Promise<WriteResult>)

Once we do that, we can chain the result of this error callback and block on the retry attempt in the flush call.

If we don't do this, we have to somehow ensure that all error callbacks have fired once we process the results. This would be a bit tricky and may not port to Java or other languages:

error.reject(...);
setImmediate(() => {
  // This only works in Node, but due to the way that ticks are processed in the VM, we can use a `setImmediate`
  // to ensure that our code only runs once all Promise chains are processed.
});

Should these be two different functions?

Please address.

Please move this line below all other import. This will help us pretend that we care about import statement order.

done :)

Could pendingOps be an array of writeBatchIndex to deferred (and not a list of these elements)?

done.

My last effort cleanup attempt:

Let me know what you think.

post discussion: The main issue with this implementation is that if the user error callback throws an error, the deferred operation will never be resolved. The double try/catch blocks, while ugly, serve to ensure that the deferred operation is always removed.