New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Drop custom Promises and refactor to async
functions
#845
Drop custom Promises and refactor to async
functions
#845
Conversation
async
functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to remove documentation for Body.Promise
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍 😍 😍
The only thing is that premature closure now silently resolves with the partial text instead of with an error, I think that we need to fix that before we merge this
test/main.js
Outdated
@@ -601,8 +601,9 @@ describe('node-fetch', () => { | |||
return fetch(url).then(res => { | |||
expect(res.status).to.equal(200); | |||
expect(res.ok).to.be.true; | |||
return expect(res.text()).to.eventually.be.rejectedWith(Error) | |||
.and.have.property('message').includes('Premature close'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, this should still reject with an error, right? Seems like the new code is missing to check for errors at some point? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's Draft PR, not ready for review yet 😄
But yes, I'm struggling to understand expected behavior with a partial body. Do we need to put a partial body somewhere or just discard it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, sorry, I was a bit eager 😁
I think that we can just discard it, since the request have errored 🤔
async
functionsasync
functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In readme:
Use native promise, but allow substituting it with [insert your favorite promise library].
should update docs
Fixes #844 and drops custom Promises.
This PR didn't complete the refactoring of
index.js
intoasync
function (as it way more complicated and time-consuming task), but it can be done by iterations later on as non-breaking changes, now when we drop support for custom promises.No changes to tests, except for the removal of testing for custom promises.