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
[vis/requesting] only call when actually requesting #14017
Conversation
1a1fa17
to
b4fae2b
Compare
b4fae2b
to
4455170
Compare
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.
LGTM
@@ -41,7 +53,7 @@ describe('SegmentedRequestProvider', () => { | |||
|
|||
function mockSource() { | |||
return { | |||
get: sinon.stub().returns(mockIndexPattern()) | |||
get: sinon.stub().returns(mockIndexPattern()), |
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.
Why the extra comma here?
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 added a line below it at one point and deleted it later
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 this makes sense and is an improvement.
$scope.searchSource.onRequestStart(() => { | ||
return $scope.vis.requesting(); | ||
}); | ||
|
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 this can be shortened to $scope.searchSource.onRequestStart($scope.vis.requesting);
?
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.
Calling vis.requesting without it's this
context might work, but seems like an unnecessary risk.
|
||
return Promise | ||
.map(this._requestStartHandlers, fn => fn(this, request)) | ||
.then(_.noop); |
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.
How come you need this .then(_.noop)
part?
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.
Documented the method as returning Promise<undefined>
and realized that it was actually returning Promise<Array<WhateverStartHandlersReturn>>
, so I used _.noop
to actually resolve to undefined
.
* [vis/requesting] only call when actually requesting * [courier/dataSource] use Promise.map to catch sync errors * [courier/request] move source mutation logic out of request (cherry picked from commit b0dc246)
* [vis/requesting] only call when actually requesting * [courier/dataSource] use Promise.map to catch sync errors * [courier/request] move source mutation logic out of request (cherry picked from commit b0dc246)
* [vis/requesting] only call when actually requesting * [courier/dataSource] use Promise.map to catch sync errors * [courier/request] move source mutation logic out of request
* [vis/requesting] only call when actually requesting * [courier/dataSource] use Promise.map to catch sync errors * [courier/request] move source mutation logic out of request
Was talking with @nreese and learned that
vis.requesting()
is being called in multiple places because the hook is actually triggered on serialization, which happens multiple times in some cases. This makes the relationship betweenvis.requesting()
and the search source more explicit by adding adataSource.onRequestStart()
hook which is called before any courier request is started.