Clean up SearchSource and get rid of AbstractDataSourceProvider #16932
Conversation
stacey-gammon
commented
Feb 27, 2018
stacey-gammon
commented
- Merge search source and abstract
- es6ify SearchSource class
stacey-gammon
force-pushed
the
search-source-abstract-cleanup
branch
from
b015f59
to
39c3046
Compare
💚 Build Succeeded |
💚 Build Succeeded |
stacey-gammon
force-pushed
the
search-source-abstract-cleanup
branch
from
1e9be7a
to
8bc30fe
Compare
💚 Build Succeeded |
|
|
||
| index(indexPattern) { | ||
| const state = this._state; | ||
|
|
There was a problem hiding this comment.
Seems like there shouldn't be an empty line here?
There was a problem hiding this comment.
Yea agree this looks weird. It was just part of the original code. :)
| delete state.source; | ||
| } | ||
|
|
||
| if (indexPattern === undefined) return state.index; |
There was a problem hiding this comment.
This distinction between undefined and null seems brittle to me. One returns the index, and one deletes it, but null and undefined are often treated as synonymous by developers. :/
There was a problem hiding this comment.
So strange indeed. I haven't had a chance to really investigate why this is, but don't want to get into it for this PR. As long as I haven't changed any logic in this PR, my only goal is to get rid of the unnecessary abstraction and turn into es6 style class.
|
|
||
| // Return promises created by the completion handler so that | ||
| // errors will bubble properly | ||
| return req.getCompletePromise().then(addRequest); |
There was a problem hiding this comment.
This appears to be an infinite loop? Not sure if it is or not. If not, is there a way to make this clearer?
There was a problem hiding this comment.
This is some funky logic for sure. Take a look at #16641 specifically this comment - #16641 (comment). I'm pretty sure this function is only used by discover and it's how it keeps a request always in the queue so auto refresh works.
We def want to clean this up (or just rewrite all the logic). Courier is some seriously old code that is pretty tricky to refactor because of all the subtle ways it's used.
For this current PR, I'd prefer to keep it simply the combining of the abstract class and switching to es6 styles (and i think I removed an unused function). I think it will be easier to isolate bugs if the PRs are relatively focused. But def a next step will be to do more refactoring/clean up here.
| * @return {[type]} [description] | ||
| */ | ||
| toString() { | ||
| return angular.toJson(this.toJSON()); |
There was a problem hiding this comment.
Couldn't we JSON.stringify here? If so, let's do that and drop one angular dependency.
There was a problem hiding this comment.
There is actually one thing different. angular.toJson removes any variables that start with $. I think that's it actually. There was a bug we ran into once because of a swap. But I agree, we should get rid of the angular dependency. I think there might be a native toJSON that does the same thing with the $... I'll have to look later.
But prefer not to put that in this PR.
| */ | ||
| _mergeProp(state, val, key) { | ||
| if (typeof val === 'function') { | ||
| const source = this; |
There was a problem hiding this comment.
Stopping the review here, as I realize you're just moving code around for the purpose of refactoring, and this code is already in the codebase. That said, I'd flag this as something that would benefit additional refactoring at some point. It's fairly difficult to follow. At ~700 loc, this feels like a "god class". Not sure what to do about it or whether it'll ever be worth it, but a lot of this appears as if it'd benefit from a conversion from OOP to a more functional approach, as it's conflating data transforms with a lot of other things (mutating the DOM, etc), so separating those things would be nice.
There was a problem hiding this comment.
Agree completely! Something I've been wanting to do forever is clean this code up but it's tricky and I've been taking very small steps to ensure forward progress (if I try to tackle too much at once I get down the rabbit hole and the changes become too huge and difficult to find the reason for bugs).
|
@chrisdavies - I'm going to go ahead and merge as is because I wanted to keep all the original logic the same. Def agree with your feedback, just think it belongs in a different PR. |
…tic#16932) * Merge search source and abstract and es6ify SearchSource class * Remove duplicate function and rename SourceAbstracts to SearchSource
…) (#17106) * Merge search source and abstract and es6ify SearchSource class * Remove duplicate function and rename SourceAbstracts to SearchSource
