Continue request --> got migration; affects [travis wordpress myget jenkins-plugin node nuget] #7215
Conversation
chris48s
added
service-badge
| }) | ||
| ) | ||
| } | ||
| const { buffer } = await checkErrorResponse({})( |
There was a problem hiding this comment.
I decided to replace the bespoke error handling here with just checkErrorResponse(). I don't think it was really buying us anything
There was a problem hiding this comment.
i think that's reasonable, assuming we don't have a large bucket somewhere of these intermediate errors that are providing helpful context (I don't think I've ever seen one)
| @@ -1,93 +1,62 @@ | |||
| import requestModule from 'request' | |||
There was a problem hiding this comment.
This file is the last file left in core/legacy/ and it has no unit tests although it is not completely untested because the service tests for all the services in the title exercise this code.
My instinct is that having migrated this to got and async/await we should move this out of core/legacy/ and write a proper test suite for it. Before I go ahead with that, it would be useful to understand why this is in legacy. Maybe @paulmelnikow can chime in on this one. Does this sufficiently modernise it, or is there another reason we want to ditch this?
There was a problem hiding this comment.
I'm going to go ahead and merge this so we can get #7214 resolved, but it would be useful to get some input on this anyway and I will follow up
| regularUpdateCache[url] = { timestamp, data } | ||
| cb(null, data) | ||
| }) | ||
| const data = scraper(reqData) |
There was a problem hiding this comment.
it's a small diff but admittedly my eyes are having difficulty following it, though curious if there's any potential behavioral concerns since the scraper call site is no longer happening within a try block?
There was a problem hiding this comment.
Yeah this took me a while to unpick too.
You'll notice that the old regularUpdate function takes a callback function as a param (cb) in the sig, but when we're invoking regularUpdate() we are not actually passing it a callback. This is because we're wrapping it in promisify() from the standard library. One of the things this does is it passes our function a callback that rejects the promise if we pass it an error
So
function regularUpdate(params, cb) {
try {
// do a thing
} catch (e) {
cb(e)
return
}
}is there to catch any exception we throw and pass it into the callback function, but its not actually handling it pre-se - its just allowing us to catch it when we resolve the promise (maybe "bubbling" it is the right word for this?). i.e: it allows us to do:
try:
await promisify(regularUpdate)(params)
} catch (e) {
// either handle the error cleanly, or ignore it and leave it sentry
}Now that regularUpdate is an async function, we can safely just throw in an async function, so
function regularUpdate(params, cb) {
try {
// do a thing
} catch (e) {
cb(e)
return
}
}in callback-land can just be replaced with
async function regularUpdate(params) {
// do a thing
}in async/await-land because:
try:
await regularUpdate(params)
} catch (e) {
// either handle the error cleanly, or ignore it and leave it sentry
}is already a thing.
There was a problem hiding this comment.
got it, thanks for the explanation!
| }) | ||
| ) | ||
| } | ||
| const { buffer } = await checkErrorResponse({})( |
There was a problem hiding this comment.
i think that's reasonable, assuming we don't have a large bucket somewhere of these intermediate errors that are providing helpful context (I don't think I've ever seen one)
Closes #7214
Refs #4655
This PR migrates
regularUpdate()from request and callbacks to got and async/await and all the knock-on effects of that.