[Discover] Migrate angular context app controllers to react #100530

dimaanj · 2021-05-25T07:52:50Z

Part of #92573

Summary

Data fetching of context app migrated from angular to react
UI action handlers moved to react
Removed redundant angular controller logic, keept only router resolver
Now users can toggle columns using the doc viewer table
Changing filters, successor and predecessor count no longer reloads the URL (And with that there's no more memory leak caused by the query bar)

ContextAppContent renders 7 times initially

DiscoverGrid/DocTable renders 4 times initially

each ActionBar component renders 3 times initially

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

…ts-migration # Conflicts: # src/plugins/discover/public/application/angular/context/query_parameters/actions.test.ts # src/plugins/discover/public/application/components/context_error_message/context_error_message.test.tsx # src/plugins/discover/public/application/components/context_error_message/context_error_message.tsx

…ange sorting, highlight anchor doc, rename legacy variables

…gination support

dimaanj · 2021-05-28T12:16:28Z

@elasticmachine merge upstream

kertal

WIP review, currently trying to compare master vs this branch Kibana in terms of performance, now my computer is stuck, guess I need a new one ... so here are my first comments. So far congrats, it's a big improvement + step forward! One thing that I noticed is ContextApp and ContextAppContent are rendered 9 time initially, it's clear hat they have to bei re-rendered serveral time due to multiple serial data fetching operation, but there might be a potential to reduce renderings a bit e.g. ContextAppContent re-renders because the property row changes, but comparing new and old rows, content is the same.

src/plugins/discover/public/application/components/context_app/utils/clamp.ts

src/plugins/discover/public/application/components/context_app/use_context_app_state.ts

kertal · 2021-06-10T07:56:55Z

src/plugins/discover/public/application/components/context_app/use_context_app_state.ts

+
+  useEffect(() => {
+    const unsubscribeAppState = stateContainer.appState.subscribe((newState) => {
+      stateContainer.flushToUrl(true);


isn't flushing to Url redundant? if there are changes in state they should be propagated to Url automatically?

This functional test needs flushing url. The line await browser.goBack(); there will not gonna do redirect to discover page if we do not flushing the url

@dmitriynj Had a closer look at it, works, but with a few changes calling this function could be omitted (it was implemented for testing). So in context we do not want to push to history like in Discover Main, we want to replace in history. If we would change the number of successor, predecessors 3 times, we would need 4 back button clicks to come back to Discover Main.

This can be solved in context_state.ts by 2 changes like

Line 192

stateStorage.set(APP_STATE_URL_KEY, mergedState, { replace: true });

Line 207

stateStorage.set(GLOBAL_STATE_URL_KEY, { filters: globalFilters }, { replace: true });

Line 216

const nextState = { ...appStateContainer.getState(), ...{ filters: appFilters } }; stateStorage.set(APP_STATE_URL_KEY, nextState, { replace: true });

Then

stateContainer.flushToUrl(true);

will no longer be necessary 🥳

Checked again in the old version, and there filters are pushed to history, not replaced. So the only change necessary should be in Line 192

kertal · 2021-06-10T08:08:53Z

src/plugins/discover/public/application/components/context_app/use_context_app_fetch.tsx

+      if (!tieBreakerField) {
+        setState(createError(statusKey, FailureReason.INVALID_TIEBREAKER));
+        toastNotifications.addDanger({
+          title: errorTitle,


we can remove the check for the tieBreakerField, without having an anchor, it makes no sense to fetch the surrounding documents (info of the anchor record is needed).

src/plugins/discover/public/application/components/context_app/context_app.tsx

kertal · 2021-06-10T10:10:22Z

@timroes note that this PR fixes a memory leak when loading more documents 🥳 , since the route in no longer reloaded on every change!

…/context_app.tsx Co-authored-by: Matthias Wilhelm <[email protected]>

dimaanj · 2021-06-10T16:07:07Z

@elasticmachine merge upstream

dimaanj · 2021-06-14T07:13:12Z

@elasticmachine merge upstream

majagrubic

Had a quick look at the code, haven't tested it yet. Some (nitpicky) comments below.

src/plugins/discover/public/application/angular/context.html

src/plugins/discover/public/application/angular/context/components/action_bar/action_bar.tsx

src/plugins/discover/public/application/components/context_app/context_app.test.tsx

majagrubic · 2021-06-14T09:36:25Z

src/plugins/discover/public/application/components/context_app/context_app.test.tsx

+        get: (key: string) => {
+          if (key === CONTEXT_TIE_BREAKER_FIELDS_SETTING) {
+            return ['_doc'];
+          } else if (key === SEARCH_FIELDS_FROM_SOURCE) {


why is this being set to true here? Would this test not work with SEARCH_FIELDS_FROM_SOURCE set to the default value, ie. false?

Tests will work when SEARCH_FIELDS_FROM_SOURCE will set to false, since testing method doesn't depend on this setting

majagrubic · 2021-06-14T09:38:21Z

src/plugins/discover/public/application/components/context_app/context_app.tsx

+  const { uiSettings: config, capabilities, indexPatterns, navigation, filterManager } = services;
+
+  const isLegacy = useMemo(() => config.get(DOC_TABLE_LEGACY), [config]);
+  const useNewFieldsApi = useMemo(() => !config.get(SEARCH_FIELDS_FROM_SOURCE), [config]);


Are there really performance optimizations in using memo here?

In case of omitting memo here, on each render useNewFieldsApi and isLegacy settings will be retrived from uiSettings

majagrubic · 2021-06-14T09:46:56Z

src/plugins/discover/public/application/components/context_app/use_context_app_fetch.tsx

+      setState(createError('anchorStatus', FailureReason.UNKNOWN, error));
+      toastNotifications.addDanger({
+        title: errorTitle,
+        text: toMountPoint(<MarkdownSimple>{error.message}</MarkdownSimple>),


Since this is displayed to the user, let's log the error message and put something more user-friendly in the UI.

Assume we can't really provide more user friendly message to the UI, since it's unknown error which might be caused bad network, bad es response... one meaningful message we already providing is localised errortitle

src/plugins/discover/public/application/components/context_app/use_context_app_fetch.tsx

majagrubic · 2021-06-14T09:55:42Z

src/plugins/discover/public/application/components/context_app/use_context_app_fetch.test.ts

+        predecessorCount: 2,
+        successorCount: 2,
+      },
+      useNewFieldsApi: false,


can this be set to true?

It can be set to true but it not really affect the resault of the tests since the entire '../../angular/context/api/context' and '../../angular/context/api/anchor' modules were mocked in the top of the file and they will return hits not depending on the useNewFieldsApi.

src/plugins/discover/public/application/components/context_app/context_app.tsx

dimaanj · 2021-06-16T11:22:37Z

@elasticmachine merge upstream

kibanamachine · 2021-06-16T13:25:58Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	368	360	-8

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`discover`	416.9KB	411.9KB	-5.1KB

History

💚 Build #130810 succeeded 1421458
💚 Build #130542 succeeded 48b99fa
💚 Build #130529 succeeded 2b53b32
💛 Build #130234 was flaky 105d04a
💚 Build #130154 succeeded 5c0d8a2

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

cc @dmitriynj

kertal

Code LGTM, tested locally with Chrome, Firefox, Safari, works as expected. Great work 👍 , this is a big step in our effort to remove Angular! Thanks for the fine tuning in terms of re-rendering
Note that you should add 2 more improvements to the description of this PR:

Now users can toggle columns using the doc viewer table (didn't work before)
Changing filters, successor and predecessor count no longer reloads the URL (And with that there's no more memory leak caused by the query bar)

majagrubic

Tested briefly in Chrome on Mac OSX, looks good to me. I trust @kertal has done a more thorough pass in testing.

…100530) * [Discover] migrate remaining context files from js to ts * [Discover] get rid of any types * [Discover] replace constants with enums, update imports * [Discover] use unknown instead of any, correct types * [Discover] skip any type for tests * [Discover] add euiDataGrid view * [Discover] add support dataGrid columns, provide ability to do not change sorting, highlight anchor doc, rename legacy variables * [Discover] update context_legacy test and types * [Discover] update unit tests, add context header * [Discover] update unit and functional tests * [Discover] remove docTable from context test which uses new data grid * [Discover] update EsHitRecord type, use it for context app. add no pagination support * [Discover] resolve type error in test * [Discover] move fetching methods * [Discover] complete fetching part * [Discover] remove redundant controller * [Discover] split up context state and components * [Discover] revert redundant css class * [Discover] rename component * [Discover] revert to upstream changes * [Discover] return upstream changes * [Discover] refactoring, context test update * [Discover] add tests for fetching methods, remove redundant files * [Discover] remove redundant angular utils, add filter test * [Discover] refactor error feedback * [Discover] fix functional test * [Discover] provide defaultSize * [Discover] clean up code * [Discover] fix eslint error * [Discover] fiix context settings * [Discover] return tieBreaker field check * [Discover] optimize things * [Discover] optimize rerenders * Update src/plugins/discover/public/application/components/context_app/context_app.tsx Co-authored-by: Matthias Wilhelm <[email protected]> * [Discover] resolve comments * [Discover] replace url instead of pushing to history. refactoring Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Matthias Wilhelm <[email protected]> # Conflicts: # src/plugins/discover/public/application/angular/context/query_parameters/state.ts

…#102503) * [Discover] migrate remaining context files from js to ts * [Discover] get rid of any types * [Discover] replace constants with enums, update imports * [Discover] use unknown instead of any, correct types * [Discover] skip any type for tests * [Discover] add euiDataGrid view * [Discover] add support dataGrid columns, provide ability to do not change sorting, highlight anchor doc, rename legacy variables * [Discover] update context_legacy test and types * [Discover] update unit tests, add context header * [Discover] update unit and functional tests * [Discover] remove docTable from context test which uses new data grid * [Discover] update EsHitRecord type, use it for context app. add no pagination support * [Discover] resolve type error in test * [Discover] move fetching methods * [Discover] complete fetching part * [Discover] remove redundant controller * [Discover] split up context state and components * [Discover] revert redundant css class * [Discover] rename component * [Discover] revert to upstream changes * [Discover] return upstream changes * [Discover] refactoring, context test update * [Discover] add tests for fetching methods, remove redundant files * [Discover] remove redundant angular utils, add filter test * [Discover] refactor error feedback * [Discover] fix functional test * [Discover] provide defaultSize * [Discover] clean up code * [Discover] fix eslint error * [Discover] fiix context settings * [Discover] return tieBreaker field check * [Discover] optimize things * [Discover] optimize rerenders * Update src/plugins/discover/public/application/components/context_app/context_app.tsx Co-authored-by: Matthias Wilhelm <[email protected]> * [Discover] resolve comments * [Discover] replace url instead of pushing to history. refactoring Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Matthias Wilhelm <[email protected]> # Conflicts: # src/plugins/discover/public/application/angular/context/query_parameters/state.ts

dimaanj and others added 22 commits May 3, 2021 10:55

[Discover] migrate remaining context files from js to ts

6e26af1

[Discover] get rid of any types

fae2360

Merge branch 'master' into js-ts-migration

5dd8d10

[Discover] replace constants with enums, update imports

5c9b64a

[Discover] use unknown instead of any, correct types

dbd8859

[Discover] skip any type for tests

902473a

[Discover] add euiDataGrid view

89f10ba

Merge branch 'master' into eui-data-grid-surr-docs

d90750b

[Discover] add support dataGrid columns, provide ability to do not ch…

35abe5b

…ange sorting, highlight anchor doc, rename legacy variables

[Discover] update context_legacy test and types

63493d5

Merge branch 'master' into eui-data-grid-surr-docs

0060ed0

Merge branch 'master' into eui-data-grid-surr-docs

a6e5d65

[Discover] update unit tests, add context header

18ad181

[Discover] update unit and functional tests

233be90

[Discover] remove docTable from context test which uses new data grid

8780907

Merge branch 'master' into eui-data-grid-surr-docs

c49026b

Merge branch 'master' into eui-data-grid-surr-docs

3a70b98

[Discover] update EsHitRecord type, use it for context app. add no pa…

6388555

…gination support

[Discover] resolve type error in test

720b613

Merge branch 'master' into eui-data-grid-surr-docs

6411879

[Discover] move fetching methods

b81a4e2

dimaanj added the Feature:Discover Discover Application label May 25, 2021

dimaanj requested a review from kertal May 25, 2021 07:52

dimaanj self-assigned this May 25, 2021

[Discover] complete fetching part

7f69c92

dimaanj force-pushed the migrate-angular-ctrls branch from 8ccd752 to 7f69c92 Compare May 28, 2021 11:47

kibanamachine and others added 2 commits May 28, 2021 08:16

Merge branch 'master' into migrate-angular-ctrls

f6049d6

[Discover] remove redundant controller

43a9966

dimaanj added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0 labels Jun 9, 2021

ryankeairns approved these changes Jun 9, 2021

View reviewed changes

kertal reviewed Jun 10, 2021

View reviewed changes

dimaanj and others added 3 commits June 10, 2021 18:42

[Discover] optimize rerenders

2b53b32

Update src/plugins/discover/public/application/components/context_app…

ec7a51d

…/context_app.tsx Co-authored-by: Matthias Wilhelm <[email protected]>

[Discover] resolve comments

19dc507

Merge branch 'master' into migrate-angular-ctrls

48b99fa

Merge branch 'master' into migrate-angular-ctrls

1421458

majagrubic reviewed Jun 14, 2021

View reviewed changes

kertal reviewed Jun 15, 2021

View reviewed changes

src/plugins/discover/public/application/components/context_app/context_app.tsx Outdated Show resolved Hide resolved

[Discover] replace url instead of pushing to history. refactoring

19e6840

Merge branch 'master' into migrate-angular-ctrls

694e17c

kertal approved these changes Jun 17, 2021

View reviewed changes

majagrubic approved these changes Jun 17, 2021

View reviewed changes

dimaanj merged commit 0cfd04c into elastic:master Jun 17, 2021

dimaanj mentioned this pull request Jun 17, 2021

[7.x] [Discover] Migrate angular context app controllers to react (#100530) #102503

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Migrate angular context app controllers to react #100530

[Discover] Migrate angular context app controllers to react #100530

dimaanj commented May 25, 2021 •

edited

Loading

dimaanj commented May 28, 2021

kertal left a comment

kertal Jun 10, 2021

dimaanj Jun 10, 2021

kertal Jun 15, 2021 •

edited

Loading

kertal Jun 15, 2021

kertal Jun 10, 2021

kertal commented Jun 10, 2021

dimaanj commented Jun 10, 2021

dimaanj commented Jun 14, 2021

majagrubic left a comment

majagrubic Jun 14, 2021

dimaanj Jun 16, 2021

majagrubic Jun 14, 2021

dimaanj Jun 14, 2021 •

edited

Loading

majagrubic Jun 14, 2021

dimaanj Jun 14, 2021 •

edited

Loading

majagrubic Jun 14, 2021

dimaanj Jun 16, 2021 •

edited

Loading

dimaanj commented Jun 16, 2021

kibanamachine commented Jun 16, 2021

kertal left a comment

majagrubic left a comment

[Discover] Migrate angular context app controllers to react #100530

[Discover] Migrate angular context app controllers to react #100530

Conversation

dimaanj commented May 25, 2021 • edited Loading

Summary

Checklist

dimaanj commented May 28, 2021

kertal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kertal Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kertal commented Jun 10, 2021

dimaanj commented Jun 10, 2021

dimaanj commented Jun 14, 2021

majagrubic 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

dimaanj Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimaanj Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimaanj Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

dimaanj commented Jun 16, 2021

kibanamachine commented Jun 16, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kertal left a comment

Choose a reason for hiding this comment

majagrubic left a comment

Choose a reason for hiding this comment

dimaanj commented May 25, 2021 •

edited

Loading

kertal Jun 15, 2021 •

edited

Loading

dimaanj Jun 14, 2021 •

edited

Loading

dimaanj Jun 14, 2021 •

edited

Loading

dimaanj Jun 16, 2021 •

edited

Loading