Update to Got 9, rename endpoint option, and require Node.js 8
#29
Conversation
index.js
Outdated
| 'head', | ||
| 'delete' | ||
| ]; | ||
| if (options.method && options.method.toLowerCase() === 'put' && !options.body) { |
There was a problem hiding this comment.
Don't we normalize the casing of options.method? If not, we should.
There was a problem hiding this comment.
No. I'll send a PR.
index.js
Outdated
| ghGot[x] = (url, opts) => ghGot(url, Object.assign({}, opts, {method})); | ||
| ghGot.stream[x] = (url, opts) => ghGot.stream(url, Object.assign({}, opts, {method})); | ||
| } | ||
| return next(options).catch(err => { |
There was a problem hiding this comment.
Can you make the handler function async and use await and normal try/catch here?
There was a problem hiding this comment.
That's not currently possible, because got assumes this is a sync function.
Oh, wait. It returns a Promise anyway :P
There was a problem hiding this comment.
Nah. That breaks streams.
There was a problem hiding this comment.
I don't see how that would break streams. Can you push what you changed?
There was a problem hiding this comment.
I don't see how that would break streams.
If handler is async, it'll return a Promise and not a stream.
Can you push what you changed?
Are you still sure?
There was a problem hiding this comment.
Oh, I had assumed handler could be an async function too, like the beforeRequest handler. I think the handler should be able to be an async function. I see many use-cases where you want to do some async stuff there.
There was a problem hiding this comment.
Yeah, that'd be very nice. But there's another problem: need to do a wrapper for .cancel() and .on(). We could accept handler also as an object containing two properties: streamHandler and promiseHandler.
It'd look like:
// Your handler:
async function promiseHandler(options, next, wrap) {
const promise = next(options);
wrap(promise); // You always need to do this :(
try {
return await promise;
} catch (err) {
// Modify `err` here
throw err;
}
}
// create.js (got)
...
const createWrapper = handler => promise => {
handler.on = promise.on;
handler.cancel = promise.cancel;
};
function got(url, options) {
try {
options = mergeOptions(defaults.options, options);
const handler = defaults.handler(normalizeArguments(url, options, defaults), next, createWrapper(handler));
return handler;
} catch (error) {
return Promise.reject(error);
}
}
...A better solution would be to create a new hook: onError. You could modify that there.
sindresorhus
changed the title
endpoint option, and require Node.js 8
|
You have to update the docs about the |
index.js
Outdated
| throw err; | ||
| }); | ||
| } | ||
| if (options.method && options.method === 'PUT' && !options.body) { |
There was a problem hiding this comment.
Assumes sindresorhus/got#547 is merged.
|
ping. any updates? |
index.js
Outdated
|
|
||
| module.exports = ghGot; | ||
| module.exports = create(); | ||
| module.exports.recreate = create; |
There was a problem hiding this comment.
If this is only for testing, it should be behind a if (process.env.TEST) { (you'll need ava@1.0.0-beta.6 for this) check, if it's for users too, it should be documented with a use-case.
There was a problem hiding this comment.
Isn't that process.env.NODE_ENV === 'test'?
There was a problem hiding this comment.
Yes, I meant process.env.NODE_ENV === 'test'...
There was a problem hiding this comment.
It's done already :)
Isn't this blocked by sindresorhus/got#547? |
Yup. I just wanted to know if there's anything else to do :) (thanks for the review) |
index.js
Outdated
|
|
||
| const url = /^https?/.test(path) ? path : opts.endpoint + path; | ||
| token: process.env.GITHUB_TOKEN, | ||
| baseUrl: process.env.GITHUB_ENDPOINT ? process.env.GITHUB_ENDPOINT.replace(/[^/]$/, '$&/') : 'https://api.github.com/', |
There was a problem hiding this comment.
This can be simplified to:
baseUrl: process.env.GITHUB_ENDPOINT || 'https://api.github.com'
package.json
Outdated
| "is-plain-obj": "^1.1.0" | ||
| }, | ||
| "devDependencies": { | ||
| "ava": "*", | ||
| "ava": "^1.0.0-beta.6", | ||
| "get-stream": "^3.0.0", |
There was a problem hiding this comment.
Also update AVA and get-stream.
| } | ||
| if (options.method && options.method === 'PUT' && !options.body) { | ||
| options.headers['content-length'] = 0; | ||
| } |
There was a problem hiding this comment.
I think we should move to handle this in Got itself.
See:
- PUT should always send Content-Length header psf/requests#1050
- https://stackoverflow.com/questions/1233372/is-an-http-put-request-required-to-include-a-body
What do you think?
index.js
Outdated
| }); | ||
| } | ||
| return next(options).catch(err => { | ||
| if (err.response && isPlainObj(err.response.body)) { |
There was a problem hiding this comment.
Do we really need to check if body is a plain object? I think we can just do if (err.response && err.response.body) {.
|
In case you missed it: #29 (comment) |
Fixes #24
Fixes #25