refactor: migrate to hapi 18 #36
Conversation
This PR refactors the HTTP API for use with Hapi 18 in preparation for ipfs/js-ipfs#1844. License: MIT Signed-off-by: Alan Shaw <[email protected]>
License: MIT Signed-off-by: Alan Shaw <[email protected]>
License: MIT Signed-off-by: Alan Shaw <[email protected]>
ghost
assigned 
| } | ||
| }) | ||
| readableStream.once('error', (error) => { | ||
| reject(error) |
There was a problem hiding this comment.
What would happen here if an error occurred during writing (so we'd already resolved the promise)?
There was a problem hiding this comment.
A promise can only be resolved/rejected once, and first one wins, so nothing really. Hapi would have already started streaming to the client. At that point we've handed over responsibility of responding (by passing the stream to hapi) and my expectation would be for either Hapi or Node.js to handle an error in the stream and close the connection...
Previously we'd have called reply again, which definitely wouldn't have worked as the HTTP headers would have already been sent and possibly some data as well.
There was a problem hiding this comment.
We should probably at least log the error then as presumably it would get swallowed.
There was a problem hiding this comment.
Happy to do this later though.
|
Released as |
This PR refactors the HTTP API for use with Hapi 18 in preparation for ipfs/js-ipfs#1844.
The current code works with Hapi 16. This is now two major versions behind the current version and we need to update our code to keep up to date with the latest bug and security fixes and performance improvements.
Hapi 17 introduced significant breaking changes, along with a switch from a callback API to a promises API hence the code changes to support this are also significant.
Note the following additional changes:
async/awaitwithtry/catchover.then/.catchfor easier to understand and more succinct code - Hapi 17+ uses this syntax and it's supported in Node.js since version 8.