Skip to content
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

Add abort support to the returned request #188

Merged
merged 3 commits into from
Feb 24, 2012
Merged

Conversation

itay
Copy link
Contributor

@itay itay commented Feb 24, 2012

This is a beginning to a solution for issue #59, to add the ability to abort an inflight request.

I doubt this change is sufficient, but it's the beginning of the direction I was thinking, and wanted to get feedback. I've tested all three cases locally:

  1. Aborting a request before it is even issued.
  2. Aborting a request before a response comes back.
  3. Aborting a request while a response is being sent.

All three worked. I have not added any tests to the pull request, because I wanted to get feedback first, but I will before it is merged.

@mikeal
Copy link
Member

mikeal commented Feb 24, 2012

.abort() should be a prototype method that checks for this.req and this.response.

abort() should not emit an error.

@itay
Copy link
Contributor Author

itay commented Feb 24, 2012

Fair enough on the first phone.

What should it do? Whoever asked to be notified regarding resolution of the request needs to know that it is no longer functioning, IMO. This also keeps it in line with what XHR (and as such, jQuery) does when abort() is invoked.

@itay
Copy link
Contributor Author

itay commented Feb 24, 2012

OK, fixed per feedback. What's left:

  • Tests
  • Formatting - not sure if it matches your current formatting or not

mikeal added a commit that referenced this pull request Feb 24, 2012
Add abort support to the returned request
@mikeal mikeal merged commit c6187dc into request:master Feb 24, 2012
@itay
Copy link
Contributor Author

itay commented Feb 24, 2012

I guess no tests necessary?

@mikeal
Copy link
Member

mikeal commented Feb 24, 2012

i want this in, nao! :)

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.

None yet

2 participants