alpacahq / alpaca-ts

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

Revert "couple of fixes " #54

Closed 117 closed 3 years ago

117 commented 3 years ago

When an error is encountered (such as unauthorized) an empty object is returned with undefined fields like so...

{
  code: 40110000,
  message: 'access key verification failed : verification failure',
  raw: [Function: raw],
  buying_power: undefined,
  regt_buying_power: undefined,
  daytrading_buying_power: undefined,
  cash: undefined,
  created_at: Invalid Date,
  portfolio_value: undefined,
  multiplier: undefined,
  equity: undefined,
  last_equity: undefined,
  long_market_value: undefined,
  short_market_value: undefined,
  initial_margin: undefined,
  maintenance_margin: undefined,
  last_maintenance_margin: undefined,
  sma: undefined,
  status: undefined
}

The error should be thrown and .catch()-able.

117 commented 3 years ago

@husayt found this issue, can't fix tonight but will take a stab tomorrow if you haven't already

husayt commented 3 years ago

@117 yes, that needs to change:

Probably I misunderstood what the return should be in case of error.

  try {
      const resp = await func()
      if (!isJson) return resp.ok as any
      let result = {}
      try {
        result = await resp.json()
      } catch (e) {
        console.warn('problem turning res to json', resp, e)
      }

      if ('code' in resp && 'message' in resp) throw Error('another problem')
      return result as any
    } catch (e) {
      console.warn('problem turning res to json', e)
    }

What should we change this to? What do we expect in case of error?

husayt commented 3 years ago

@117 from going through the old code this is what I ended up with:

      let resp
      try {
        resp = await func()
      } catch (e) {
        throw(e)
      }
      if (!isJson) return resp.ok as any
      let result = {}
      try {
        result = await resp.json()
      } catch (e) {
        throw(result)
      }

      if ('code' in result && 'message' in result ) throw result 
      return result as any

That is why I wanted to move away from promises in first place, it's very confusing what that code does. Still I am not sure if this is the correct async variant

117 commented 3 years ago

Can you open a new PR from your fork?