Migrate SO management libs to typescript #60555

pgayvallet · 2020-03-18T19:19:24Z

Summary

'low effort' straightforward migration of the src/legacy/core_plugins/kibana/public/management/sections/objects/lib code base to typescript

PR's goal is to ease the diff of the next PR(s) by pre-migrating the management lib functions to TS. Behavior and implementation remains unchanged.

Did only minimal adaptation of the tests and did not add any, as we will wait to migrate the libs to NP apis before doing so.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-03-18T19:20:53Z

Pinging @elastic/kibana-platform (Team:Platform)

…gement-lib-to-ts

pgayvallet · 2020-03-19T08:08:48Z

...ore_plugins/kibana/public/management/sections/objects/lib/__jest__/get_relationships.test.ts

-    const $http = jest.fn();
+    const $http = jest.fn() as any;


$http a callable class. Too difficult to properly type even with jest utilities. This is going to be removed anyway when migrating to NP apis

Is this file going to be removed?

not the file, but the $http service is going to be replaced by core's http service, which provide proper mocks.

I'm asking because in the NP we keep test files side by side to the code, while here it's under a separate __jest__ folder.

Wasn't sure about the jest test-files pattern for legacy, but it seems I can safely move them one level up. Will do.

pgayvallet · 2020-03-19T08:10:10Z

...re_plugins/kibana/public/management/sections/objects/lib/__jest__/import_legacy_file.test.js

-  it('should import a file', async () => {
-    class FileReader {
-      readAsText(text) {
-        this.onload({
-          target: {
-            result: JSON.stringify({ text }),
-          },
-        });
-      }
-    }
+  it('should import a file with valid json format', async () => {
+    const file = new File([`{"text": "foo"}`], 'file.json');

-    const file = 'foo';
-
-    const imported = await importLegacyFile(file, FileReader);
-    expect(imported).toEqual({ text: file });
+    const imported = await importLegacyFile(file);
+    expect(imported).toEqual({ text: 'foo' });


Was using a mocked FileReader implementation, therefor testing only a small part of the importFile function. Adapted the tests to use a real File as input

pgayvallet · 2020-03-19T08:11:03Z

...ugins/kibana/public/management/sections/objects/lib/__jest__/process_import_response.test.ts

-          obj: {
-            type: 'a',
-            id: '1',
-          },
+          type: 'a',
+          id: '1',


Test dataset did not match real structure. Adapted test data and updated snapshots.

pgayvallet · 2020-03-19T08:13:00Z

...plugins/kibana/public/management/sections/objects/lib/__jest__/resolve_saved_objects.test.ts

-jest.mock('../../../../../../../../../plugins/kibana_utils/public', () => ({
-  SavedObjectNotFound: class SavedObjectNotFound extends Error {
-    constructor(options) {
-      super();
-      for (const option in options) {
-        if (options.hasOwnProperty(option)) {
-          this[option] = options[option];
-        }


This mocking was useless, as the only runtime usage of the mock was the import in the test file... adapted to declare the SavedObjectNotFound in the test file instead

pgayvallet · 2020-03-19T08:14:55Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/import_legacy_file.ts

-export async function importLegacyFile(file, FileReader = window.FileReader) {
+export async function importLegacyFile(file: File) {


the FileReader param was only there for testing purposes. Adapted the tests and removed the param.

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/jest/import_legacy_file.test.js is still js file

pgayvallet · 2020-03-19T08:16:24Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/resolve_saved_objects.ts

-        values: { title: this.title },
+        values: { title },


Seems like an error after a probable extraction from a react component. I don't even know how the this reference was working (probably it wasn't).

pgayvallet · 2020-03-19T08:18:52Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/resolve_saved_objects.ts


-async function getSavedObject(doc, services) {
-  const service = services.find(service => service.type === doc._type);
+type SavedObjectsRawDoc = Record<string, any>;


Couldn't find a proper type for the raw doc we are manipulating in this file. Even the server side SavedObjectsRawDoc doesn't match. Had to fallback to a generic record...

pgayvallet · 2020-03-19T08:25:18Z

src/legacy/core_plugins/kibana/public/management/sections/objects/types.ts

+export interface SavedObjectMetadata {
+  icon?: string;
+  title?: string;
+  editUrl?: string;
+  inAppUrl?: { path: string; uiCapabilitiesPath: string };
+}
+
+export type SavedObjectWithMetadata<T = unknown> = SavedObject<T> & {
+  meta: SavedObjectMetadata;
+};
+
+export interface SavedObjectRelation {
+  id: string;
+  type: string;
+  relationship: 'child' | 'parent';
+  meta: SavedObjectMetadata;
+}


Duplicated types with src/plugins/saved_objects_management. Will be removed once everything get moved.

mshustov · 2020-03-19T11:28:09Z

...ore_plugins/kibana/public/management/sections/objects/lib/__jest__/get_relationships.test.ts

-    const $http = jest.fn();
+    const $http = jest.fn() as any;


Is this file going to be removed?

mshustov · 2020-03-19T11:34:50Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/find_objects.ts

  const response = await kfetch({
    method: 'GET',
    pathname: '/api/kibana/management/saved_objects/_find',
-    query: findOptions,
+    query: findOptions as Record<string, any>,


KFetchQuery type is not really useful 😕

Yea. all kfetch usages will be removed when this moves to the NP plugin, so I did not really bother much about these force-casts.

mshustov · 2020-03-19T11:36:59Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/get_relationships.ts

-    const response = await $http(options);
-    return response ? response.data : undefined;
+    const response = await $http<SavedObjectRelation[]>(options);
+    return response?.data;
  } catch (resp) {


that's something new

mshustov · 2020-03-19T11:38:24Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/import_legacy_file.ts

-export async function importLegacyFile(file, FileReader = window.FileReader) {
+export async function importLegacyFile(file: File) {


src/legacy/core_plugins/kibana/public/management/sections/objects/lib/jest/import_legacy_file.test.js is still js file

mshustov · 2020-03-19T11:41:36Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/in_app_url.ts

  switch (type) {
    case 'search':
    case 'searches':
-      return uiCapabilities.discover.show;
+      return uiCapabilities.discover.show as boolean;


I'm surprised that we don't allow to specify the type of Capabilities via a generic interface

Yes, this is something that was 'migrated' a little too fast to TS imho. Adding a genetic type on Capabilities would probably allow to remove quite a lot of force-cast in our TS plugins code.

However something like Capabilities<T> extends T is not permitted, so I'm not sure how we would allow to add arbitrary root-level properties on Capabilities to replace

/** Custom capabilities, registered by plugins. */ [key: string]: Record<string, boolean | Record<string, boolean>>;

mshustov · 2020-03-19T11:45:14Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/is_same_query.ts


-export const isSameQuery = (query1, query2) => {
+export const isSameQuery = (query1: any, query2: any) => {


I cannot see we use it. I would rather remove the code. We always can find removed code in the git history.

Yea, only usage is in a TODO (🙈)

kibana/src/legacy/core_plugins/kibana/public/management/sections/objects/components/objects_table/objects_table.js

Lines 240 to 242 in 7df981f

onQueryChange = ({ query }) => {

// TODO: Use isSameQuery to compare new query with state.activeQuery to avoid re-fetching the

// same data we already have.

I'll remove it for now

mshustov · 2020-03-19T11:48:08Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/resolve_saved_objects.ts

+  doc: SavedObjectsRawDoc,
+  indexPatterns: IndexPatternsContract,
+  overwriteAll: boolean,
+  confirmModalPromise: OverlayStart['openConfirm']


Is it a method or a promise? Signature says that's a method. Let's rename to something more meaningful - openConfirm, for example

mshustov · 2020-03-19T11:51:59Z

src/legacy/core_plugins/kibana/public/management/sections/objects/lib/resolve_saved_objects.ts

-  conflictedIndexPatterns,
-  overwriteAll
+  resolutions: Array<{ oldId: string; newId: string }>,
+  conflictedIndexPatterns: any[],


working with code in this file is not fun 😓

lib/resolve_saved_objects.js and lib/resolve_import_errors.js really could use some proper cleanup and refactoring (most of the lib function does). I will create an issue to track all possible improvements once most of the SO management PR lands.

…gement-lib-to-ts

spalger · 2020-03-26T08:04:15Z

@elasticmachine merge upstream

…gement-lib-to-ts

kibanamachine · 2020-03-26T13:40:42Z

💚 Build Succeeded

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

History

💚 Build #36466 succeeded 371813a
💚 Build #36431 succeeded 99f4ab9
💔 Build #36416 failed 4f4ed59
💚 Build #35290 succeeded d6cbad4
💚 Build #34979 succeeded cea0c72

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* migrate most libs * migrate last lib files * fix get_relationships * fix getSavedObject * migrate tests to TS * address review comments * move test files outside of __jest__ folder

pgayvallet added 3 commits March 18, 2020 15:07

migrate most libs

f31a193

migrate last lib files

1109601

fix get_relationships

0bfea64

pgayvallet added Feature:New Platform release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.7.0 v8.0.0 labels Mar 18, 2020

pgayvallet added 3 commits March 19, 2020 08:00

fix getSavedObject

70f1973

migrate tests to TS

9303b9f

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

7d3d1f2

…gement-lib-to-ts

pgayvallet marked this pull request as ready for review March 19, 2020 08:07

pgayvallet requested a review from a team as a code owner March 19, 2020 08:07

pgayvallet commented Mar 19, 2020

View reviewed changes

pgayvallet requested a review from a team March 19, 2020 08:26

mshustov approved these changes Mar 19, 2020

View reviewed changes

pgayvallet added 6 commits March 19, 2020 14:17

address review comments

7439d10

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

36aa682

…gement-lib-to-ts

move test files outside of __jest__ folder

e1c0717

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

f4a3b1d

…gement-lib-to-ts

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

cea0c72

…gement-lib-to-ts

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

d6cbad4

…gement-lib-to-ts

mattkime approved these changes Mar 23, 2020

View reviewed changes

pgayvallet added 2 commits March 26, 2020 07:40

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

4f4ed59

…gement-lib-to-ts

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

99f4ab9

…gement-lib-to-ts

pgayvallet added v7.8 and removed v7.7.0 labels Mar 26, 2020

pgayvallet added 2 commits March 26, 2020 10:52

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

371813a

…gement-lib-to-ts

Merge remote-tracking branch 'upstream/master' into kbn-50308-so-mana…

c618664

…gement-lib-to-ts

pgayvallet merged commit cec9165 into elastic:master Mar 26, 2020

pgayvallet mentioned this pull request Mar 26, 2020

[7.x] Migrate SO management libs to typescript (#60555) #61459

Merged

rayafratkina added v7.8.0 and removed v7.8 labels Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate SO management libs to typescript #60555

Migrate SO management libs to typescript #60555

pgayvallet commented Mar 18, 2020 •

edited

Loading

elasticmachine commented Mar 18, 2020

pgayvallet Mar 19, 2020

mshustov Mar 19, 2020

pgayvallet Mar 19, 2020

mshustov Mar 20, 2020

pgayvallet Mar 20, 2020

pgayvallet Mar 19, 2020

pgayvallet Mar 19, 2020

pgayvallet Mar 19, 2020

pgayvallet Mar 19, 2020

mshustov Mar 19, 2020

pgayvallet Mar 19, 2020

pgayvallet Mar 19, 2020

pgayvallet Mar 19, 2020

mshustov Mar 19, 2020

mshustov Mar 19, 2020

pgayvallet Mar 19, 2020

mshustov Mar 19, 2020

mshustov Mar 19, 2020

mshustov Mar 19, 2020

pgayvallet Mar 19, 2020

mshustov Mar 19, 2020

pgayvallet Mar 19, 2020

mshustov Mar 19, 2020

mshustov Mar 19, 2020

pgayvallet Mar 19, 2020

spalger commented Mar 26, 2020

kibanamachine commented Mar 26, 2020

		export async function importLegacyFile(file, FileReader = window.FileReader) {
		export async function importLegacyFile(file: File) {


		export const isSameQuery = (query1, query2) => {
		export const isSameQuery = (query1: any, query2: any) => {

	onQueryChange = ({ query }) => {
	// TODO: Use isSameQuery to compare new query with state.activeQuery to avoid re-fetching the
	// same data we already have.

Migrate SO management libs to typescript #60555

Migrate SO management libs to typescript #60555

Conversation

pgayvallet commented Mar 18, 2020 • edited Loading

Summary

Checklist

elasticmachine commented Mar 18, 2020

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

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

spalger commented Mar 26, 2020

kibanamachine commented Mar 26, 2020

💚 Build Succeeded

History

pgayvallet commented Mar 18, 2020 •

edited

Loading