[ML] Injectables refactor #50512

jgowdyelastic · 2019-11-13T18:19:31Z

New platform related refactor PR.
Removes the use of soon to be obsolete injectables.
Removed:
Private
kbnBaseUrl
kbnUrl
reactDirective
some uses of $scope
Removes the use of $route by SearchItemsProvider and so removes the need of a provider wrapper entirely.

Also typescriptifies and refactors some simple util files and directives.

@peteharverson special care is needed when reviewing the changes made to field_format_service. From my tests everything appears to be correct, but there may be edge cases i've missed.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-11-13T19:04:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 17b0ba31fc03bd87bab5d9375775d9ae5b704286

elasticmachine · 2019-11-14T10:27:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 40ee771507fb682eeb900afa5f9a82e78c342faf

elasticmachine · 2019-11-14T12:25:29Z

💔 Build Failed

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

elasticmachine · 2019-11-14T15:15:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 15db3f79ff9ab8d195146befc11aea23aa554c12

elasticmachine · 2019-11-14T15:26:34Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2019-11-14T15:56:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3e52b1ccf49226387a190002c49bed72b0a22228

elasticmachine · 2019-11-14T16:40:42Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5dd773627d25c44a51d94426f351751b5674bda5

elasticmachine · 2019-11-14T18:06:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6eba95ef4d8047e2e91234750a4e7e5dc7fc0d11

elasticmachine · 2019-11-14T22:10:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6a7ff8407c707f4a42e02d4bd630dc0d1a8fa15b

x-pack/legacy/plugins/ml/public/license/check_license.tsx

peteharverson · 2019-11-15T10:40:25Z

x-pack/legacy/plugins/ml/public/services/field_format_service.ts

+          resolve(this.formatsByJob);
+        })
+        .catch(err => {
+          // console.log('fieldFormatService error populating formats:', err);


Can this be removed?

x-pack/legacy/plugins/ml/public/settings/calendars/edit/directive.tsx

x-pack/legacy/plugins/ml/public/settings/calendars/list/directive.tsx

x-pack/legacy/plugins/ml/public/settings/filter_lists/edit/directive.tsx

x-pack/legacy/plugins/ml/public/settings/filter_lists/list/directive.tsx

peteharverson · 2019-11-15T10:46:57Z

x-pack/legacy/plugins/ml/public/util/index_utils.ts

@@ -62,7 +46,7 @@ export function getIndexPatternIdFromName(name: string) {
      return indexPatternCache[j].id;
    }
  }
-  return name;
+  return null;


Just to clarify, this is changing the default return type to null rather than returning the name for cases where no matching index pattern could be found. Probably makes more sense, but just want to make sure!

i checked the places where this is called and returning null makes more sense.
After this refactor, there are no places where returning the originally supplied index name is expected.

peteharverson

Tested this out, including where the field format service is using, and all looks good.

Other than the minor comments added above, LGTM

darnautov · 2019-11-15T09:24:53Z

x-pack/legacy/plugins/ml/common/util/validation_utils.ts

+import { VALIDATION_STATUS } from '../constants/validation';
+
+// get the most severe status level from a list of messages
+const contains = (arr: string[], str: string) => arr.findIndex(v => v === str) >= 0;


just migration to TS, but for an array of primitives it's easier to use arr.indexOf(str)

darnautov · 2019-11-15T09:39:31Z

x-pack/legacy/plugins/ml/public/jobs/new_job_new/utils/new_job_utils.ts

+  indexPattern: any,
+  savedSearch: any


we have interfaces for those two 🙂

darnautov · 2019-11-15T09:40:28Z

x-pack/legacy/plugins/ml/public/license/check_license.tsx

 import React from 'react';
+// @ts-ignore No declaration file for module
+import { banners } from 'ui/notify';
+// import chrome from 'ui/chrome';


commented import

darnautov · 2019-11-15T11:14:30Z

x-pack/legacy/plugins/ml/public/license/check_license.tsx

-let licenseType = null;
-let expiredLicenseBannerId;
+let licenseType: LICENSE_TYPE | null = null;
+let expiredLicenseBannerId: any;


is it possible to provide a type?

Adding type of string but it's worth noting that it's a guess because banners is js.
It's also worth noting that the value expiredLicenseBannerId is never really used, it's only checked to see if it has been set.

darnautov · 2019-11-15T11:17:40Z

x-pack/legacy/plugins/ml/public/services/field_format_service.ts

-    this.indexPatternIdsByJob = {};
-    this.formatsByJob = {};
-  }
+  indexPatternIdsByJob: any = {};


darnautov · 2019-11-15T11:20:32Z

x-pack/legacy/plugins/ml/public/services/field_format_service.ts

-      const formatsByDetector = {};
+      const formatsByDetector: any[] = [];


object replaced with an array 🤔

yes, it looks like it was incorrect being an object.
formatsByDetector is indexed using detector_index from each detector which is an incrementing integer.

darnautov · 2019-11-15T11:21:37Z

x-pack/legacy/plugins/ml/public/settings/calendars/edit/directive.tsx

  return {
    restrict: 'E',
    replace: false,
    scope: {},
-    link: function (scope, element) {
+    link(scope: any, element: ng.IAugmentedJQuery) {


ng.IScope ?

darnautov · 2019-11-15T11:21:57Z

x-pack/legacy/plugins/ml/public/settings/calendars/list/directive.tsx

  return {
    restrict: 'E',
    replace: false,
    scope: {},
-    link: function (scope, element) {
+    link(scope: any, element: ng.IAugmentedJQuery) {


ng.IScope ?

darnautov · 2019-11-15T11:22:23Z

x-pack/legacy/plugins/ml/public/settings/filter_lists/edit/directive.tsx

  return {
    restrict: 'E',
    replace: false,
    scope: {},
-    link: function (scope, element) {
+    link(scope: any, element: ng.IAugmentedJQuery) {


ng.IScope?

darnautov · 2019-11-15T11:25:39Z

x-pack/legacy/plugins/ml/public/util/index_utils.ts

-  return name;
+  return null;


is it correct?

darnautov

LGTM 👍

elasticmachine · 2019-11-15T15:43:07Z

💚 Build Succeeded

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

jgowdyelastic · 2019-11-18T08:49:50Z

@elasticmachine merge upstream

elasticmachine · 2019-11-18T08:49:52Z

merge conflict between base and head

walterra

LGTM, just added some minor comments.

walterra · 2019-11-18T08:57:08Z

x-pack/legacy/plugins/ml/common/util/__tests__/parse_interval.ts

+import expect from '@kbn/expect';
+import { parseInterval } from '../parse_interval';
+
+describe('ML parse interval util', function() {


We also need to migrate away from mocha. Since these tests don't include anything angular related, they could be moved to .../util/parse_interval.test.ts and migrated to jest.

walterra · 2019-11-18T08:58:21Z

x-pack/legacy/plugins/ml/common/util/__tests__/string_utils.ts

@@ -4,14 +4,11 @@
 * you may not use this file except in compliance with the Elastic License.
 */

-
-
 import expect from '@kbn/expect';
 import { renderTemplate } from '../string_utils';

 describe('ML - string utils', () => {


Could be moved to .../util/string_utils.test.ts and migrated to jest.

yeah, makes sense to move it over

walterra · 2019-11-18T09:00:12Z

x-pack/legacy/plugins/ml/common/util/parse_interval.ts

 import dateMath from '@elastic/datemath';

+type SupportedUnits = unitOfTime.Base;


Just wondering if this should be singular SupportedUnit since later on it's used as an array like SupportedUnits[].

unitOfTime.Base is a list of string literal types, something like ms | s | m | h |....
so from a language point of view i thought the plural made sense.
but i guess it's a union so it'll only have one value.

elasticmachine · 2019-11-18T10:18:07Z

💔 Build Failed

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

elasticmachine · 2019-11-18T10:58:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 134feed

elasticmachine · 2019-11-18T11:53:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 5a27d77

elasticmachine · 2019-11-18T14:34:26Z

💚 Build Succeeded

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

* [ML] Injectables refactor * removing unrelated files * additional typescript conversion * more typescript conversion * adding some return types * fixing eui errors * typescripting license checks * updated based on review * fixing merge conflict error * converting tests to jest * fixing types

jgowdyelastic force-pushed the injectables-refactor branch from e22c589 to 15db3f7 Compare November 14, 2019 13:58

jgowdyelastic requested review from darnautov, walterra, peteharverson and alvarezmelissa87 November 14, 2019 15:25

jgowdyelastic self-assigned this Nov 14, 2019

jgowdyelastic added :ml review v7.6.0 v8.0.0 labels Nov 14, 2019

jgowdyelastic added non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes labels Nov 14, 2019

jgowdyelastic marked this pull request as ready for review November 14, 2019 15:27

jgowdyelastic requested a review from a team as a code owner November 14, 2019 15:27

peteharverson reviewed Nov 15, 2019

View reviewed changes

peteharverson approved these changes Nov 15, 2019

View reviewed changes

darnautov reviewed Nov 15, 2019

View reviewed changes

jgowdyelastic force-pushed the injectables-refactor branch from c787b6e to f1caf40 Compare November 15, 2019 14:21

darnautov approved these changes Nov 15, 2019

View reviewed changes

jgowdyelastic added 8 commits November 18, 2019 08:58

[ML] Injectables refactor

0329523

removing unrelated files

0c39c68

additional typescript conversion

bfabc7d

more typescript conversion

8b8297d

adding some return types

7db7169

fixing eui errors

6a9a00e

typescripting license checks

4a6b0a9

updated based on review

e4726c8

jgowdyelastic force-pushed the injectables-refactor branch from f1caf40 to e4726c8 Compare November 18, 2019 09:00

walterra approved these changes Nov 18, 2019

View reviewed changes

fixing merge conflict error

134feed

converting tests to jest

5a27d77

fixing types

5032008

jgowdyelastic merged commit eb4c47e into elastic:master Nov 18, 2019

jgowdyelastic deleted the injectables-refactor branch November 18, 2019 14:36

jgowdyelastic mentioned this pull request Nov 18, 2019

[7.x] [ML] Injectables refactor (#50512) #50910

Merged

peteharverson added the Feature:NP Migration label Dec 12, 2019

		const formatsByDetector = {};
		const formatsByDetector: any[] = [];

		import dateMath from '@elastic/datemath';

		type SupportedUnits = unitOfTime.Base;

[ML] Injectables refactor #50512

[ML] Injectables refactor #50512

Conversation

jgowdyelastic commented Nov 13, 2019 • edited Loading

Checklist

For maintainers

elasticmachine commented Nov 13, 2019

💔 Build Failed

elasticmachine commented Nov 14, 2019

💔 Build Failed

elasticmachine commented Nov 14, 2019

💔 Build Failed

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

elasticmachine commented Nov 14, 2019

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peteharverson left a comment • edited Loading

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

darnautov left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 15, 2019

💚 Build Succeeded

jgowdyelastic commented Nov 18, 2019

elasticmachine commented Nov 18, 2019

walterra 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

jgowdyelastic Nov 18, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Nov 18, 2019

💔 Build Failed

elasticmachine commented Nov 18, 2019

💔 Build Failed

elasticmachine commented Nov 18, 2019

💔 Build Failed

elasticmachine commented Nov 18, 2019

💚 Build Succeeded

jgowdyelastic commented Nov 13, 2019 •

edited

Loading

peteharverson left a comment •

edited

Loading

jgowdyelastic Nov 18, 2019 •

edited

Loading