-
Notifications
You must be signed in to change notification settings - Fork 82
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
Allow Request Cancelation #287
Conversation
add cancelable to all requests
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
Thank you for the pull request! I agree with you that cancellable mode is something our SDK should support. However, I am not familiar with Angular internals and I am concerned about possible impact on existing users. Why isn't cancellable already enabled by default by Angular? What are the downsides of cancellable requests - are there situations where we don't wont the request to be cancellable? |
Also, shouldn't we enable |
The reason cancellable isn't enabled by default it seems, is due to backwards compatibility. Cancellable requests were added in v1.5 of angular's ngResource module. Prior to that, they only supported canceling requests by timeout value. This value "could" be a promise but its behavior was buggy. Since v1.4.9, $timeout has to be a pure numeric value, while promise based cancelation is supported only via cancellable: true. Post requests to end-points can be canceled, as its a part of the XHR spec, and in modern-day popular libraries (like axios) they support cancelation via promises as well. I think the behavior should support this. Adding cancellable: true to all ngResources is the only way to generate promises for them. Ideally, i think, i'd love to see an option per request to enable/disable this feature. Since there is an "options" param on most endpoints, we could use that. It might look something like this: request = Users.find({
filter: {
where: { name },
}
options: {
cancellable: true,
}
}); But i'm not entirely familiar with how to go about doing this. If you could point me in the right direction, i'd love to tackle this (and maybe add timeout configuration support as well). And to note, since the |
Thank you @hellsan631 for detailed explanation. I am ok to proceed with this pull request then!
Could you please do that as part of this pull request? Here is the place where we are defining which Angular version to fetch: loopback-sdk-angular/package.json Lines 34 to 36 in 79e8730
Please check why the tests are failing on Travis. Apparently, the build is installing the latest Angular version 1.7.0 (as specified in our package.json file, see build log), I think there is some other problem causing your patch to not work as intended.
Unfortunately, there are no options AFAICT, there are only
|
it('has a $cancelRequest property', function() { | ||
var list = MyModel.find( | ||
function() { | ||
|
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.
Please remove this empty line.
var list = MyModel.find
function() {},
util.throwHttpError
);
With the release of LoopBack 4.0 GA (see the announcement), this SDK has entered Active LTS mode which means we are no longer accepting new features (see our LTS policy). I have to close this pull request as rejected, sorry for that. |
Description
Adds the
cancellable: true
field to all generated $request entities, allowing for request cancelation.After adding this field means all returned resource objects have a function called
$cancelRequest()
which can be used to cancel a request in transit.Previously, you were not able to cancel a request generated by lbservices. This is a common pattern that should be supported, but with the addition of this PR, it is.
Example:
Checklist
guide