Typescriptify search source #47644

lukasolson · 2019-10-08T22:56:29Z

Summary

Typescriptify & jestify courier/search source.

Closes #46315.

Closes #49661

elasticmachine · 2019-11-19T21:02:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: a54281e

elasticmachine · 2019-11-20T23:24:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c0f95f4

lukasolson · 2019-11-21T15:36:07Z

@elastic/ml-ui Could I get a quick review on the ML files changed here?

alvarezmelissa87

Gave this a test and LGTM for ML 👍

ppisljar

code LGTM

lukeelmers

Overall code LGTM! I added a handful of comments, most very minor items that aren't critical to merging this. I also had a few questions on areas I'm not familiar with.

lukeelmers · 2019-11-22T17:55:38Z

src/legacy/ui/public/agg_types/buckets/terms.ts

-  getRequestInspectorStats,
-  getResponseInspectorStats,
-} from '../../courier/utils/courier_inspector_utils';
+import { getRequestInspectorStats, getResponseInspectorStats } from '../../courier';


nit: Could combine these imports from ../../courier

lukeelmers · 2019-11-22T17:57:17Z

src/legacy/ui/public/courier/fetch/errors.ts

-  public resp: any;
-  constructor(err: any, resp?: any) {
+  public resp: SearchResponse;
+  constructor(err: any, resp?: SearchResponse) {


I wonder if err could/should be typed more strictly? Should this be SearchError?

lukeelmers · 2019-11-22T18:05:43Z

src/legacy/ui/public/courier/search_source/normalize_sort_request.ts

+ * under the License.
+ */
+
+import { IndexPattern } from '../../../../core_plugins/data/public';


Probably better to use the interface exposed from the new platform plugin for this:

Suggested change

import { IndexPattern } from '../../../../core_plugins/data/public';

import { IIndexPattern } from '../../../../../plugins/data/public';

(Same probably applies to a handful of other places in this PR, I added comments wherever I saw them)

lukeelmers · 2019-11-22T18:09:00Z

src/legacy/ui/public/courier/search_source/normalize_sort_request.ts

+function normalize(
+  sortable: EsQuerySortValue,
+  indexPattern: IndexPattern | string | undefined,
+  defaultSortOptions: any


should this be defaultSortOptions: SortOptions?

No, defaultSortOptions comes from an advanced UI setting. We could probably type it but it really should come from Elasticsearch since it represents additional parameters you can send in your sort portion of a search request.

lukeelmers · 2019-11-22T18:10:29Z

src/legacy/ui/public/courier/search_source/search_source.test.ts

+ */
+
+import { SearchSource } from '../search_source';
+import { IndexPattern } from '../../../../core_plugins/data/public/index_patterns';


IIndexPattern from the NP plugin is preferred if it works here

lukeelmers · 2019-11-22T18:58:52Z

src/legacy/core_plugins/kibana/public/discover/embeddable/search_embeddable.ts

-    this.filtersSearchSource = searchSource.create();
-    this.filtersSearchSource.setParent(timeRangeSearchSource);
+    this.filtersSearchSource = searchSource.create()!;
+    this.filtersSearchSource!.setParent(timeRangeSearchSource);


I think tsc will still work without the ! on these two lines

lukeelmers · 2019-11-22T19:01:58Z

src/legacy/ui/public/courier/fetch/call_client.ts

@@ -39,15 +48,20 @@ export function callClient(searchRequests, requestsOptions = [], { es, config, e
  Object.keys(searchStrategyMap).forEach(searchStrategyId => {
    const searchStrategy = getSearchStrategyById(searchStrategyId);
    const requests = searchStrategyMap[searchStrategyId];
-    const { searching, abort } = searchStrategy.search({ searchRequests: requests, es, config, esShardTimeout });
+
+    const { searching, abort } = searchStrategy!.search({


If getSearchStrategyById could possibly return undefined, then maybe we should be adding an if (searchStrategy) {} check here, so that TS doesn't need the !

Updating getSearchStrategyById to never return undefined.

lukeelmers · 2019-11-22T19:06:11Z

src/legacy/ui/public/courier/fetch/fetch_soon.ts

+  options: FetchOptions,
+  { es, config, esShardTimeout }: FetchHandlers
+) {
+  const msToDelay = config!.get('courier:batchSearches') ? 50 : 0;


I think the ! can be removed here

lukeelmers · 2019-11-22T19:07:35Z

src/legacy/ui/public/courier/fetch/handle_response.tsx

-          request={request.body}
-          response={response}
+          request={request.body as Request}
+          response={response as ResponseWithShardFailure}


I don't believe casting is necessary here

lukeelmers · 2019-11-22T19:12:06Z

src/legacy/ui/public/courier/search_source/types.ts

+export type SearchRequest = any;
+export type SearchResponse = any;


Would Record<string, any> or Record<string, unknown> work here?

elasticmachine · 2019-11-25T21:27:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0dcc9c9

elasticmachine · 2019-11-25T23:54:08Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 2d1da99

elasticmachine · 2019-11-26T01:13:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 51c92b0

lukasolson · 2019-11-26T01:24:44Z

7.x (7.6.0): #51682

lukasolson added 30 commits September 9, 2019 20:31

Initial refactor of search source

9aa6e48

Add abort signal to search source fetch and remove cancel queued

039c507

Remove usages of Angular Promises

fb1f959

Remove usages of angular "sessionId" service

1a40af4

Merge branch 'master' into refactorSearchSource

6331629

Remove config as dependency

8aa6496

Update deps on config and esShardTimeout

027f6b6

Remove remaining angular dependencies from SearchSource

ae21ec8

Merge branch 'master' into refactorSearchSource

ac12f45

Fix Karma tests

493f882

Separate callClient and handleResponse and add tests for each

68e7529

Merge branch 'master' into refactorSearchSource

dcbcded

Add tests for fetchSoon

ca27f89

Add back search source test and convert to jest

9ff10c4

Create search strategy registry test

fa1ad58

Revert empty test

aa87511

Remove filter predicates from search source

43305ec

Merge branch 'master' into refactorSearchSource

6240738

Update typings and throw response errors

df0a353

Fix proxy to properly return response from ES

5496403

Merge branch 'master' into refactorSearchSource

2c23fe5

Update jest snapshots

41567df

Remove unused translations

c1e1ddb

Don't pass search request to onRequestStart, create it afterwards

b3e8d11

Fix search source & get search params tests

649e6fa

Merge branch 'master' into refactorSearchSource

6fb788c

Merge branch 'master' into refactorSearchSource

7c6b7b8

Merge branch 'master' into refactorSearchSource

1a44042

Fix issue with angular scope not firing after setting state on vis

cfbed7a

Fix issue with angular scope not firing after setting state on vis

0d7c97d

Merge branch 'master' into typescriptifySearchSource

a54281e

lukasolson added 5 commits November 19, 2019 15:17

Merge branch 'master' into typescriptifySearchSource

75604ec

Fix filter matches index for filters with partial meta

af3faa0

Merge remote-tracking branch 'upstream/master'

21d6cd3

Merge branch 'master' into typescriptifySearchSource

5bd959e

Fix type errors

c0f95f4

lukasolson requested a review from ppisljar November 20, 2019 23:54

lukeelmers self-requested a review November 21, 2019 15:18

alvarezmelissa87 approved these changes Nov 21, 2019

View reviewed changes

ppisljar approved these changes Nov 21, 2019

View reviewed changes

lukeelmers reviewed Nov 22, 2019

View reviewed changes

lukasolson added 3 commits November 25, 2019 11:54

Merge remote-tracking branch 'upstream/master'

d118abb

Merge branch 'master' into typescriptifySearchSource

13380ed

Fix references

0dcc9c9

lukasolson added 2 commits November 25, 2019 15:27

Address review feedback

2d1da99

Fix failing test

51c92b0

lukasolson merged commit e3a97dd into elastic:master Nov 26, 2019

lukasolson added a commit to lukasolson/kibana that referenced this pull request Nov 26, 2019

Backport elastic#47644 to 7.x

11f951d

lukasolson mentioned this pull request Nov 26, 2019

[7.x] Typescriptify search source #51682

Merged

lukasolson added a commit that referenced this pull request Nov 26, 2019

Backport #47644 to 7.x (#51682)

1910487

lukasolson deleted the typescriptifySearchSource branch December 2, 2019 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typescriptify search source #47644

Typescriptify search source #47644

lukasolson commented Oct 8, 2019 •

edited by ppisljar

Loading

elasticmachine commented Nov 19, 2019

elasticmachine commented Nov 20, 2019

lukasolson commented Nov 21, 2019

alvarezmelissa87 left a comment

ppisljar left a comment

lukeelmers left a comment

lukeelmers Nov 22, 2019

lukeelmers Nov 22, 2019

lukeelmers Nov 22, 2019

lukeelmers Nov 22, 2019

lukasolson Nov 25, 2019

lukeelmers Nov 22, 2019

lukeelmers Nov 22, 2019

lukeelmers Nov 22, 2019

lukasolson Nov 25, 2019

lukeelmers Nov 22, 2019

lukeelmers Nov 22, 2019

lukeelmers Nov 22, 2019

elasticmachine commented Nov 25, 2019

elasticmachine commented Nov 25, 2019

elasticmachine commented Nov 26, 2019

lukasolson commented Nov 26, 2019

	import { IndexPattern } from '../../../../core_plugins/data/public';
	import { IIndexPattern } from '../../../../../plugins/data/public';

		export type SearchRequest = any;
		export type SearchResponse = any;

Typescriptify search source #47644

Typescriptify search source #47644

Conversation

lukasolson commented Oct 8, 2019 • edited by ppisljar Loading

Summary

elasticmachine commented Nov 19, 2019

💔 Build Failed

elasticmachine commented Nov 20, 2019

💚 Build Succeeded

lukasolson commented Nov 21, 2019

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 25, 2019

💚 Build Succeeded

elasticmachine commented Nov 25, 2019

💔 Build Failed

elasticmachine commented Nov 26, 2019

💚 Build Succeeded

lukasolson commented Nov 26, 2019

lukasolson commented Oct 8, 2019 •

edited by ppisljar

Loading