Streaming pagination support (streamingRequest) #31
Conversation
There was a problem hiding this comment.
mmm, thank you for stepping up once again; I appreciate the help :)
I see where you're going with this, and it seems like a solid foundation for implementing all sorts of streaming — not exclusively pagination. It's a great farkin idea, and I hadn't thought of implementing pagination like this. Hell yeah, brother!
As per usual, I care a lot about the naming, ramble a bit about past decisions I made, but I am a wee bit proud about pondering whether the canceler should return a Promise. I feel like that might be a good call :P
src/interface.ts
Outdated
| type RequestMultipleHandler = (error: {response_status: number, data: any} | null, payload: {[prop: string]: any} | null) => any; | ||
|
|
||
| export | ||
| type RequestMultipleCancel = () => void; |
There was a problem hiding this comment.
I think it'd pair better with Handler to have this be RequestMultipleCanceler. And I suppose the object being described is not technically a "cancel", but a thing that causes cancellation, so a "canceler".
Also, in learning from previous mistakes, it may be wise to have this return a Promise which, in the future, could have the ability to send a "stop" request to the server before resolving. It definitely makes sense to immediately (i.e. not on the next tick) remove the listener, though, to guarantee that once cancel() is called, callback will not be invoked again.
There was a problem hiding this comment.
Added a StreamingRequestPromise to replace StreamingRequestCanceler as the return type for DCRFClient#streamingRequest in 88d1b53.
There was a problem hiding this comment.
In b90ec59, I'm being more certain that callback is not called after cancel;
src/interface.ts
Outdated
| export | ||
| type RequestMultipleHandler = (error: {response_status: number, data: any} | null, payload: {[prop: string]: any} | null) => any; | ||
|
|
||
| export | ||
| type RequestMultipleCancel = () => void; | ||
|
|
There was a problem hiding this comment.
mmm, actually, do you think streamingRequest makes more sense than requestMultiple? I'm concerned about the ambiguity of requestMultiple — like, does it make multiple requests? Is it requesting multiple objects? Or is it what it actually does: make a single request for multiple responses?
Perhaps DCRFClient.streamingRequest() with StreamingRequestHandler and StreamingRequestCanceler? (I was toying with DCRFClient.requestStreaming(), but I'm leaning toward streamingRequest.) What do you think?
There was a problem hiding this comment.
Made this name change in 635369f.
test/test.ts
Outdated
| const msg = transport.send.getCall(0).args[0]; | ||
| const stream = msg.stream; | ||
| const requestId = msg.payload.request_id; |
There was a problem hiding this comment.
This has nothing to do with this PR; I'm just now realizing how I could've done all of these three lines much more succinctly with a single destructuring assignment. This implementation I went with is so... Pythonic :P
If I wrote these tests again, I'd do it like this:
const [{stream, payload: {request_id: requestId}}] = transport.send.firstCall.args;There was a problem hiding this comment.
Made this refactor in 635369f.
src/interface.ts
Outdated
| @@ -406,6 +411,8 @@ interface IStreamingAPI { | |||
| * On failure, the promise will be rejected with the entire API response. | |||
| */ | |||
| request(stream: string, payload: object, requestId?: string): Promise<object>; | |||
|
|
|||
| requestMultiple(stream: string, payload: object, callback:RequestMultipleHandler, requestId?: string): RequestMultipleCancel; | |||
There was a problem hiding this comment.
nit: spacing
| requestMultiple(stream: string, payload: object, callback:RequestMultipleHandler, requestId?: string): RequestMultipleCancel; | |
| requestMultiple(stream: string, payload: object, callback: RequestMultipleHandler, requestId?: string): RequestMultipleCancel; |
There was a problem hiding this comment.
I'd like to see some JSDocs for the method before this gets released, but that can wait.
There was a problem hiding this comment.
Added some JSDocs for this in 635369f.
jhillacre
changed the title
|
Made those requested changes and responded to the individual comments, but not sure if that triggers a GitHub notification. (So here is a top-level comment to trigger a notification in case one wasn't generated.) |
My company ended up delivering the projects we were using dcrf on, so for now we are not wanting to take up a maintainer role. |
This PR adds a method to
DCRFClient, allowing for a single request to listening for responses until the returned cancel function is called. The intention of creating this method is to support streaming pagination fromdjangochannelsrestframework, as described in #22.