replace request with node-fetch
#13
Conversation
… stream. added example for it
| return this.requestREST(`/api/v1/fill/${pdfTemplateID}.pdf`, { | ||
| method: 'POST', | ||
| json: payload, | ||
| encoding: null, |
There was a problem hiding this comment.
All good without specifying an encoding? It doesnt try to translate into UTF8 by default?
There was a problem hiding this comment.
It is my understanding that for the request library, the encoding: null option said "I want the response to be a Buffer", which makes sense b/c we wrote the client to deal with buffers as the happy-path response.
With node-fetch, you explicitly tell it how you'd like to consume the response via the response.buffer() (or similar) calls, and there is no UTF-stuff that happens when dealing with the stream or buffer types. There is, for example, when you call response.json() to consume it.
Anywho, I think we're good and this worked for me no problem locally.
From the request documentation:
encoding- encoding to be used on setEncoding of response data. If null, the body is returned as a Buffer. Anything else (including the default value of undefined) will be passed as the encoding parameter to toString() (meaning this is effectively utf8 by default). (Note: if you expect binary data, you should set encoding: null.)
Relevant node-fetch docs: https://www.npmjs.com/package/node-fetch#interface-body
|
@benogle plz have another look now that I've updated docs and tests...ended up changing a few things. |
| @@ -1,8 +1,15 @@ | |||
| const request = require('request') | |||
| const fetch = require('node-fetch') | |||
| const defaultOptions = { | ||
| baseURL: 'https://app.useanvil.com', | ||
| userAgent: `${description}/${version}`, |
| if (!options.apiKey && !options.accessToken) throw new Error('apiKey or accessToken required') | ||
|
|
||
| this.options = Object.assign({}, defaultOptions, options) |
There was a problem hiding this comment.
for a custom supplied userAgent would we still want to tack on the /<version>? Would let us know which client version they were using. I suppose this isn't used currently though.
There was a problem hiding this comment.
We could do that...but it sort of felt to me like if the User was overriding it that we shouldn't tinker with things. We decided to leave this undocumented for now anyways.
Another thing we could consider is always adding a custom header indicating that it came from our node module.
For now, it's nice to get our client in the user agent header though!
README.md
Outdated
| baseUrl: 'https://app.useanvil.com', // Optional. String setting the base URL for all client requests | ||
| userAgent: 'Anvil API Client/<version_number>' // Optional. String setting the User-Agent header in all requests | ||
| } |
There was a problem hiding this comment.
Not sure we want to put these front and center. They probably shouldnt be used. I'd be ok if they were undocumented
Description of the change
Replace
requestwithnode-fetch. Also:User-Agentoption and default.Type of change
Related issues
Fixes #7
Checklists
Development
Code review