alpacahq / alpaca-ts

A TypeScript Node.js library for the https://alpaca.markets REST API and WebSocket streams.
ISC License
154 stars 42 forks source link

Exception while getting json() of DELETE responses #52

Closed husayt closed 3 years ago

husayt commented 3 years ago

Description There are a few methods in Alpaca api which are done through DELETE call. It is not standard for DELETE calls to return result, but here they do. The problem is when we try to do fetch().json() they throw an exception. This library currently wraps that exception and returns {}, but then we are missing the original result.

Expected To return the received response from function.

Reproduction Steps we can take to reproduce the bug: For exampe, trying to cancel an order with cancelOrder(id) you don't know what was the result.

Additional This is due to fetch handling DELETE messages (at least in a browser). Need to find a proper way of handling it

117 commented 3 years ago

Good catch.

117 commented 3 years ago

Do you think the status should be returned or the entire response?

// if json parse fails we default to status return
.then(async (resp) => (await resp.json().catch(() => false)) || reject(resp.status))
husayt commented 3 years ago

This is not really solving the problem. Exception handling is for unseen problems. When we know that certain call is always causing an error, we should better find a way to do it properly without exceptions, rather than hiding that error.

Especially for this kind of critical api, it is important to preserve and pass on everything we receive. The call is to cancel an order, what if order was not cancelled and we return ok? Consequences could be absolutely damning.

The problem we have is that response of DELETE call is not being handled properly by fetch. We should find a way to somehow extract the response.

husayt commented 3 years ago

Actually for this particular request here is what documentation say:

[DELETE] Cancel an order DELETE/v2/orders/{order_id} Attempts to cancel an open order. If the order is no longer cancelable (example: status="filled"), the server will respond with status 422, and reject the request. Upon acceptance of the cancel request, it returns status 204.

So we shouldn't even try to do json() here, as there is no body returned in response. I will come up with suggestion, also that method has mix of async and promises syntax. Will convert it all to async for better readability

husayt commented 3 years ago

fix is in #53