Add client side abort for request.js

[jung-kim]

Add client side abort function for request.js

(this does not abort severside call.)

fix #365

[wmdmark]

Looking forward to seeing this merged! Causing some headaches for my team right now.
cc @dustinlarimer

[dustinlarimer]

@wmdmark happy to check this out, but I really could use a little context on what issues this will resolve for your team. Would you mind sharing? It would be most appreciated!

[wmdmark]

@dustinlarimer having the ability to cancel a pending ajax request is useful thing to expose on it's own but the particular area where it's causing problems for us is the following scenario:

• We have learning app where a user can view stats about themselves under a tab on their profile. We have a KeenQuery React component that encapsulates the logic for fetching and rendering the stats (fetch is done with keen-js).
• If the user navigates away from the about tab before all the stats are loaded (some of them take ~10 seconds) then the KeenQuery component is unmounted when the client.run callback is resolved. This tries to update the component but it's already unmounted so throws an error. Without being able to cancel a query, there's no way for us to handle this error without resorting to the hacky, and soon to be depricated, isMounted function.

Let me know if you need any more details than that.

[dustinlarimer]

@wmdmark first off, it sounds like you're building something awesome!

I do agree that exposing an .abort() or .cancel() method would be useful in some cases, but the problem you're describing is a bit more general to handling asynchronous callbacks in the React component lifecycle. This begs the question of whether or not data should be fetched from within components, or above/outside those components with an isolated data store.

We've encountered the same issue in our own apps, and have found this sort of pub/sub model to work much better. Asynchronous actions update a data store, which then emits a change event, affecting only the mounted+subscribed components.

How does that sound? Do you think this would be a useful pattern in your project?

[wmdmark]

@dustinlarimer we do use a flux-like pattern for most things in our app but we'd like the KeenQuery component to be "drop-in" w/out having to setup a specific store keys for every instance of it. Which is why we need to be able to handle the query canceling in componentWillUnmount. While the async/pub sub pattern is better for most cases, I don't think keen-js should put developers in a situation where just using plain React/View patterns wouldn't be an option. Even React's tutorial example uses componentDidMount to call an ajax API (using jQuery $.ajax which is cancelable via abort). Just exposing the XHR from client.run would do the trick (or accepting this pull request).

[dustinlarimer]

@wmdmark totally understand!

The request object that client.run() returns includes a basic event emitter, and emits a "complete" event when a query is returned. I wonder if this could help right now, as-is:

componentDidMount: function(){
  this.req = client.run(query);
  this.req.on('complete', this.handleResponse);
},

componentWillUnmount: function(){
  this.req.off('complete');
}

Do you think that might do the trick? This won't abort the XHR request, but may prevent the error that's holding you up. I do agree that an .abort() method would be extremely useful, but I'll need a little more time to review this before we can roll it into a release.

[wmdmark]

@dustinlarimer Yep, that solution does work for us! Thanks for your help!

[dustinlarimer]

@wmdmark Awesome, I'm relieved to hear that! Sorry for the speed-bump! If you hit any other critical issues and need to chat, just give me a shout at dustin@keen.io.

[dustinlarimer]

@wmdmark If we can get this PR synced with master and updated to use .cancel() instead of .abort() I would be happy to release this!

The difference in method names may seem trivial, but keen-analysis.js promises are powered by bluebird, which exposes a .cancel() method that does exactly this for HTTP requests. Keeping the interface vocabulary and conceptual model small would be super helpful, as we'll eventually roll keen-analysis.js into a future release of keen-js (along with our other new SDK modules).