Skip to content

Conversation

@cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jun 13, 2018

This slightly simplifies the way SearchSource works. I've verified that nothing uses PromiseEmitter or passes a handler to onResults by grepping the codebase and waiting for CI to pass.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@spalger
Copy link
Contributor

spalger commented Jun 13, 2018

I appreciate that this will simplify the SearchSource, but can you please verify that nothing is using PromiseEmitter before removing it? If you did, please include details like that in the description.

@spalger
Copy link
Contributor

spalger commented Jun 13, 2018

Also, by using PromiseEmitter, I more mean relying on the fact that handlers for methods like onResults() are called multiple times. It's possible nothing is even passing a handler to onResults(), but it'd be great to know that type of thing before going into review.

@cjcenizal
Copy link
Contributor Author

Thanks @spalger! Done.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Checked queries in discover, visualize, dashboard, and updates on filter and time range.

Good catch and thanks for doing this. Any opportunity to rip complexity out of the courier is an opportunity we should take.

Copy link
Contributor

@jen-huang jen-huang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Verified that there aren't any other usages of PromiseEmitter or passing a handler to onResults.

@cjcenizal cjcenizal merged commit 7a9d4a2 into elastic:master Jun 13, 2018
@cjcenizal cjcenizal deleted the remove-on-results-handler branch June 13, 2018 17:34
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Jun 13, 2018
maryia-lapata pushed a commit to maryia-lapata/kibana that referenced this pull request Jun 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants