-
Notifications
You must be signed in to change notification settings - Fork 12
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
Streaming pagination support (streamingRequest) #31
Changes from 5 commits
f1a3dc7
d5ef552
24ce331
665b57a
2fbf973
635369f
88d1b53
b90ec59
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,6 +11,11 @@ type DispatchListener<T> = (response: T) => any; | |||||
export | ||||||
type SubscriptionHandler = (payload: {[prop: string]: any}, action: string) => any; | ||||||
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mmm, actually, do you think Perhaps There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made this name change in 635369f. |
||||||
/** | ||||||
* Calls all handlers whose selectors match an incoming payload. | ||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: spacing
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added some JSDocs for this in 635369f. |
||||||
} | ||||||
|
||||||
|
||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -243,6 +243,114 @@ describe('DCRFClient', function() { | |
}); | ||
}); | ||
|
||
describe('requestMultiple', function() { | ||
it('sends request and listen for responses until cancel', function () { | ||
const responses: any[] = []; | ||
const cancel = api.requestMultiple('test', {'key': 'unique'}, (error, response) => { | ||
responses.push(response); | ||
}); | ||
|
||
expect(transport.send).to.have.been.calledOnce; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made this refactor in 635369f. |
||
|
||
transport.emit('message', { | ||
data: { | ||
stream, | ||
payload: { | ||
request_id: requestId, | ||
response_status: 200, | ||
data: {response: 'unique'} | ||
} | ||
} | ||
}); | ||
|
||
transport.emit('message', { | ||
data: { | ||
stream, | ||
payload: { | ||
request_id: requestId, | ||
response_status: 200, | ||
data: {response: 'unique2'} | ||
} | ||
} | ||
}); | ||
|
||
cancel(); | ||
|
||
transport.emit('message', { | ||
data: { | ||
stream, | ||
payload: { | ||
request_id: requestId, | ||
response_status: 200, | ||
data: {response: 'unique3'} | ||
} | ||
} | ||
}); | ||
|
||
expect(responses).to.deep.equal([{'response': 'unique'}, {'response': 'unique2'}]); | ||
}); | ||
|
||
it('cancels when receiving an error.', function () { | ||
const responses: any[] = []; | ||
const errors: any[] = []; | ||
const cancel = api.requestMultiple('test', {'key': 'unique'}, (error, response) => { | ||
if (error) { | ||
errors.push(error); | ||
} else { | ||
responses.push(response); | ||
} | ||
}); | ||
|
||
expect(transport.send).to.have.been.calledOnce; | ||
const msg = transport.send.getCall(0).args[0]; | ||
const stream = msg.stream; | ||
const requestId = msg.payload.request_id; | ||
|
||
transport.emit('message', { | ||
data: { | ||
stream, | ||
payload: { | ||
request_id: requestId, | ||
response_status: 200, | ||
data: {response: 'unique'} | ||
} | ||
} | ||
}); | ||
|
||
transport.emit('message', { | ||
data: { | ||
stream, | ||
payload: { | ||
request_id: requestId, | ||
response_status: 400, | ||
data: {response: 'unique2'} | ||
} | ||
} | ||
}); | ||
|
||
transport.emit('message', { | ||
data: { | ||
stream, | ||
payload: { | ||
request_id: requestId, | ||
response_status: 200, | ||
data: {response: 'unique3'} | ||
} | ||
} | ||
}); | ||
|
||
expect(responses).to.deep.equal([{'response': 'unique'}]); | ||
expect(errors).to.deep.equal([{ | ||
request_id: requestId, | ||
response_status: 400, | ||
data: {response: 'unique2'} | ||
}]); | ||
expect(cancel).to.not.throw(); | ||
}); | ||
}); | ||
|
||
describe('subscribe', function() { | ||
it('invokes callback on every update', 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.
I think it'd pair better with
Handler
to have this beRequestMultipleCanceler
. 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 oncecancel()
is called,callback
will not be invoked again.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.
Added a
StreamingRequestPromise
to replaceStreamingRequestCanceler
as the return type forDCRFClient#streamingRequest
in 88d1b53.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.
In b90ec59, I'm being more certain that callback is not called after cancel;