Skip to content

Conversation

leiming
Copy link

@leiming leiming commented May 11, 2017

The request in reqs[uid] may not get a standard instance of request while calling customRequest function which returns a Promise.

Checking abort function before being called could solve this scenario.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9abb8a9 on leiming:master into ** on react-component:master**.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling dcca820 on leiming:master into ** on react-component:master**.

@benjycui
Copy link
Member

According to me, custom request should support abort.

@leiming
Copy link
Author

leiming commented May 12, 2017

Thank you for replying me, @benjycui

If we will support to customizing customAbort function while calling customRequest , there is no reason not to support to customizing other refer request functions which include onSuccess, onError or onProgress.

I think that abort function bind in request instance is also a feasible scheme.

@paranoidjk
Copy link
Member

@leiming

you can help us modify the api doc, the customRequest should be () => { abort: Function }

and IMO, this PR is acceptable, more robust check before call.

@benjycui
Copy link
Member

and IMO, this PR is acceptable, more robust check before call.

Nope, connection should be close when we don't need them.

e.g. we are uploading a big file(50MB?), then we destroy Upload. We should destroy connection, otherwise, this will occupy the network.

@benjycui
Copy link
Member

I think this PR should be closed. @shepherdwind

you can help us modify the api doc, the customRequest should be () => { abort: Function }

cc @leiming , and you can add a warning, if the return value of customRequest is invalid.

@paranoidjk
Copy link
Member

ok,check and warning before call is the way too,that means we think abort is not a optional interface

@benjycui
Copy link
Member

benjycui commented May 17, 2017

@paranoidjk sometimes, we can swallow an error, but I don't think this error should be swallowed. So, just let it crash and print a warning, for this problem can be found easily while developing.

@leiming
Copy link
Author

leiming commented May 18, 2017

@paranoidjk @benjycui , I agree on the error should not be swallowed since abort interface is not optional but required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants