feat: make Request object thenable #515
Conversation
pablopalacios
force-pushed
the
make-it-thenable
branch
2 times, most recently
from
8bc6d7c to
a5ecd22
Compare
In this way we don't need to call .end() to trigger the request when using promises.
pablopalacios
force-pushed
the
make-it-thenable
branch
from
a5ecd22 to
5965fc4
Compare
| .params({id: ###}) | ||
| .end(function (err, data, meta) { | ||
| // handle err and/or data returned from data fetcher in this callback | ||
| .params({ id: 42 }) |
There was a problem hiding this comment.
This is more of a style comment and less about the code changes, but I've never been a fan of this programming style. All of this code was written before async / await was available. I would prefer if the API were something simpler like:
try {
// read(/* service */, /* params */, /* config */);
const data = await fetcher.read('data_service', { id: 42 });
} catch (e) {
console.log(e);
}Maybe this is something we change for a 1.x release.
There was a problem hiding this comment.
Yup, I agree. I only fixed the docs because there are people at my company requiring a more up to date docs. I've also revisited what we discussed on #275 and this is what I want to tackle next. I also think it's possible to make it backward compatible so we can adjust everything before going to v1 and remove all the other APIs.
There was a problem hiding this comment.
Makes sense to me. BC is ideal but not a blocker if it makes the API better and user friendly.
| }, delay); | ||
| }); | ||
| } | ||
|
|
||
| function io(options, controller) { | ||
| function _fetch() { | ||
| var self = this; |
There was a problem hiding this comment.
nit: do we still need to use self?
There was a problem hiding this comment.
We need it later in the fetch call itself. Most of the time we could use this and only self in the fetch promise. But having both would be annoying so I opted to only use one. I was also very tempted to use arrow functions so we wouldn't need this workaround. I was afraid that you would consider it a breaking change, so I kept the es5 style.
| } | ||
|
|
||
| function delayPromise(fn, delay) { | ||
| // _retry is the onReject promise callback that we attach to the |
There was a problem hiding this comment.
Any thing to review in these changes or is it just reorganizing the code?
There was a problem hiding this comment.
Not really, just reorganizing.
It's mainly refactoring. Calling |
|
@redonkulus any blocker? |
Now the request object returned by fetchr crud methods are thenable by default.
To make it possible, I had to refactor
httpRequestto transform it in a class to make it easier to exposethen,catchandabortto the users. By doing it, it was also possible to simplify theRequestclass and remove some workarounds related toabort.This PR also implement some of the stuff we discussed in #263 (comment) without the need for a major version bump.
Before
After
This is also quite nice when doing fire and forget calls with async/await:
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.