heroku client bugs and missing features

[jdx]

as part of this, the functionality to make multiple requests to fill up content with range headers needs to be ported

• [x] range headers
• [x] 2fa retry

cli-engine-command

• [ ] HEROKU_DEBUG=1 no longer dumps the response body
• [ ] HEROKU_DEBUG_HEADERS=1 PR
• [ ] HEROKU_DEBUG & HEROKU_DEBUG_HEADERS does not dump response body & headers

cli-engine-heroku

• [ ] HEROKU_HOST does not allow api.heroku.com override (in Heroku)
• [ ] HEROKU_HOST with alternate port would not be respected
• [ ] HEROKU_HTTP_PROXY_HOST (maybe)
• [ ] HEROKU_HTTP_PROXY_PORT (maybe)
• [ ] HEROKU_HTTP_PROXY_AUTH (maybe)
• [x] partial (to disable fetching all pages)
• [ ] support both auth and token
• [ ] auth options should not prepend : but token should

http-call

• [ ] parseJSON (to disable JSON parsing) (maybe)
• [ ] rejectUnauthorized
• [ ] remove null | undefined headers
• [ ] request signature does not match

return heroku.request({
  path: `/apps/${app}/sni-endpoints`,
  headers: {'Accept': 'application/vnd.heroku+json; version=3.sni_ssl_cert'}
})

• [ ] addhandleFailure & handleResponse method (we override handleFailure to do 2fa)
• [ ] missing json: false request body serialization disabling
• [ ] conditionally add displaying of POST body
• [ ] set Content-length to zero when no body
• [ ] add timeout to options (maybe)
• [ ] add middleware functionality
• [ ] does not handle response code 304
• [ ] add logger to options (maybe)
• [ ] return {} if body is empty (maybe)
• [ ] add parseJSON option & always json parse? (maybe)
• [ ] missing verb instance methods with paths - new Heroku('https://api.heroku.com').get('/apps')
• [ ] do not retry 400 level errors

TODO

• [x] range headers
• [x] 2fa retry

[jdx]

for now we can continue to use heroku-cli-util/heroku-client until we're ready to move to http-call

[garyposter]

Jeff has added PATCH PUT and DELETE; and added the header debug. We need the Range header to work, and to handle Reason, Unauthorized and 2fa bits. 2fa will be big.

Philipe also found a bug in how we do Heroku authorization in V6.

[ransombriggs]

@garyposter I finished going through the node-heroku-client and added updated the missing features TODO with things that I noticed.

[jdx]

This is great, thanks for going through. I think you've gone through and listed just everything that's different, but I'd like to make a case for some of these that we don't need or shouldn't include some of them.

@ransombriggs why do we need to support auth and token?

the 2fa stuff (like handleFailure) I don't think is necessary since 2fa is in now: https://github.com/heroku/cli-engine-heroku/blob/master/src/api_client.js#L59

I don't think parseJSON stuff is necessary since http-call looks at the content type to determine whether or not to parse. You could use .stream() or raw if you really wanted to disable it.

My goal is not to make it fully compatible. I don't want to add .path to request. It would be somewhat difficult to merge the options that way and I feel people overuse the path option when it's always uglier code when it is used.

Middleware is not necessary anymore since I intend people to extend the class instead to add functionality.

304 does not need to be handled (I don't think) as http-call does not support caching (and I don't think ever should)

I don't want the logger in place, that's handled via inheritance

"missing verb instance methods with paths - new Heroku('https://api.heroku.com').get('/apps')" not sure I understand. This is possible with inheritance: https://github.com/heroku/cli-engine-heroku/blob/master/src/api_client.js#L50. Could it be cleaner? yes, but making that work for anything other than hosts would be hard, so I would encourage the inheritance method since you can change any of the behavior in a general way.

"do not retry 400" http-call doesn't retry on any response, not just 4xx. I am on the fence whether or not it is a good idea to retry 5xx.