[data.ui] Lazy load UI components in data plugin. #78889

lukeelmers · 2020-09-29T23:35:55Z

In an effort to reduce the initial bundle size of the data plugin, this updates items exported from data/public/ui to be lazy-loaded.

There's more we could investigate in the data plugin longer term, but this is the quickest/easiest target for bundle size reduction that doesn't involve breaking any service contracts or making lifecycle methods async.

Here are the before and after stats from webpack-bundle-analyzer:

Before: `data/public/ui` ~307k

Data Plugin

Closeup of data/public/ui

After: `data/public/ui` ~37k

Data Plugin

Closeup of data/public/ui

lukeelmers · 2020-10-01T21:25:34Z

src/plugins/data/public/ui/filter_bar/filter_editor/lib/filter_label.test.tsx

 import { phraseFilter } from '../../../../stubs';

-afterEach(cleanup);


These cleanups should no longer be needed as explained in #78742

lukeelmers · 2020-10-01T21:27:44Z

src/plugins/data/public/ui/index.ts

-// @internal
-export { ShardFailureOpenModalButton, ShardFailureRequest } from './shard_failure_modal';
-
-// @internal
-export { SavedQueryManagementComponent } from './saved_query_management';
-
-// @internal
-export { SaveQueryForm, SavedQueryMeta } from './saved_query_form';


data/public/index.ts imports from this file, so I have removed any items which are imported internally by other lazy-loaded components. This way the only items available in ui/index are types, or lazy-loaded components.

lukeelmers · 2020-10-01T21:30:32Z

src/plugins/data/public/ui/query_string_input/query_string_input.test.tsx

+    await waitFor(() => getByText(kqlQuery.query));
+    await waitFor(() => getByText('KQL'));


waitFor already fails if the items you are querying for don't exist, so we no longer need the assertions that were here previously.

lukeelmers · 2020-10-01T21:32:06Z

src/plugins/data/public/ui/search_bar/search_bar.test.tsx

  const SEARCH_BAR_ROOT = '.globalQueryBar';
-  const FILTER_BAR = '.filterBar';
+  const FILTER_BAR = '.globalFilterBar';


TBH I have no clue how this test worked previously with the .filterBar class... I couldn't find it anywhere in the DOM. So I replaced it with .globalFilterBar which definitely exists 🙂

lukeelmers · 2020-10-01T21:40:53Z

x-pack/plugins/security_solution/public/common/components/query_bar/index.test.tsx

+  // The data plugin's `SearchBar` is lazy loaded, so we need to ensure it is
+  // available before we mount our component with Enzyme.
+  const getWrapper = async (Component: ReturnType<typeof Proxy>) => {
+    const { getByTestId } = render(Component);
+    await waitFor(() => getByTestId('queryInput')); // check for presence of query input
+    return mount(Component);
+  };


React.Suspense doesn't play nice with Enzyme, but I wanted to avoid re-writing each test with @testing-library/react since I'm not familiar with this area of code. So I made this wrapper which ensures the SearchBar that's loaded with React.lazy is available before Enzyme mount is called.

You may want to consider a rewrite with @testing-library/react that doesn't rely on <SearchBar /> internals, as it should speed these tests up a bit. (Or perhaps a functional test if you really want to test the end-to-end experience).

elasticmachine · 2020-10-02T03:38:03Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

lukeelmers · 2020-10-02T14:05:35Z

After several attempts, I've been unable to reproduce these failures locally, however they are failing consistently so I'll keep investigating.

[00:07:24]                 └- ✖ fail: maps app  blended vector layer should request documents when zoomed to smaller regions showing less data
[00:07:24]                 │      Error: expected '34' to equal '33'
[00:07:24]                 │       at Assertion.assert (/dev/shm/workspace/parallel/24/kibana/packages/kbn-expect/expect.js:100:11)
[00:07:24]                 │       at Assertion.equal (/dev/shm/workspace/parallel/24/kibana/packages/kbn-expect/expect.js:227:8)
[00:07:24]                 │       at Context.it (test/functional/apps/maps/blended_vector_layer.js:30:23)

mattkime

Changes look good and work well locally. Nice work!

FrankHassanabad

LGTM

ThomThomson

Code only review - but Kibana App changes LGTM

lukeelmers · 2020-10-02T22:51:48Z

I haven't had any luck getting to the bottom of this failed maps functional test, so I've pinged @elastic/kibana-gis for input:

[00:07:24]                 └- ✖ fail: maps app  blended vector layer should request documents when zoomed to smaller regions showing less data
[00:07:24]                 │      Error: expected '34' to equal '33'

Since this test doesn't appear to have been failing on other branches lately, I'm trying to figure out what I changed that might have affected this test. The two components which are now lazy loaded that I could find usage of in the maps app are SearchBar and IndexPatternSelect.

One hypothesis I had was that perhaps the SearchBar was loading more slowly in CI, causing the map to temporarily render at a larger size and thus include a wider area in the query, resulting in the one additional hit that comes back (34 instead of 33).

I did a few things to try to reproduce the failure locally:

Tried throttling network speed.
Tried manually delaying rendering of the SearchBar and IndexPatternSelect.
Tried removing the SearchBar completely to see if it changed the results.
Looked at the PageObjects.maps methods to see if there were any obvious places that race conditions might occur.

In all cases I was still unable to reproduce the failure 🙁

The screenshot from the test artifacts doesn't show any obvious rendering problems either:

At this point I've adjusted the height of the SearchBar fallback component to see if it helps (so that the UI doesn't "jump" as that component loads).

But if this next run still fails, I might need some more expertise from @elastic/kibana-gis.

lukeelmers · 2020-10-05T22:40:29Z

x-pack/test/functional/apps/maps/blended_vector_layer.js

+      // Allow a range of hits to account for variances in browser window size.
+      expect(parseInt(hits)).to.be.within(30, 40);


Discussed with @nreese, and he agreed that in this case it would be fine to make this assertion a range of values to account for small variances in screen size.

kibanamachine · 2020-10-06T04:27:27Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 47525ce

Metrics [docs]

@kbn/optimizer bundle module count

id	before	after	diff
`data`	569	570	+1

async chunks size

id	before	after	diff
`data`	0.0B	273.8KB	⚠️ +273.8KB

distributable file count

id	before	after	diff
`default`	48278	48308	+30
`oss`	29048	29078	+30

page load bundle size

id	before	after	diff
`data`	1.3MB	1.1MB	-248.8KB

History

💚 Build #79941 succeeded acda6d9
💔 Build #79363 failed 9f98cd83958ad6b2ed987fa4587fc9e57a345bd5
💔 Build #79034 failed 3ee8d4d39ead4966a62c8ca6a417d02cd1175389
💔 Build #78958 failed d003dfe82aeb98c63dfc3135e9d565f8a0a4dda1
💔 Build #78135 failed 69fed127762b450c5469d65d587a6aa0cd8f7fa8

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

nreese

maps changes LGTM
code review

…into feature/task_manager_429 * 'feature/task_manager_429' of github.com:elastic/kibana: (158 commits) Add license check to direct package upload handler. (#79653) [Ingest Manager] Rename API /api/ingest_manager => /api/fleet (#79193) [Security Solution][Resolver] Simplify CopyableField styling and add comments (#79594) Fine-tunes ML related text on Metrics UI (#79425) [ML] DF Analytics creation wizard: ensure job creation possible when model memory lower than estimate (#79229) Add new "Add Data" tutorials (#77237) Update APM telemetry docs (#79583) Revert "Add support for runtime field types to mappings editor. (#77420)" (#79611) Kibana request headers (#79218) ensure missing indexPattern error is bubbled up to error callout (#79378) Missing space fix (#79585) remove duplicate tab states (#79501) [data.ui] Lazy load UI components in data plugin. (#78889) Add generic type params to search dependency. (#79608) [Ingest Manager] Internal action for policy reassign (#78493) [ILM] Add index_codec to forcemerge action in hot and warm phases (#78175) [Ingest Manager] Update open API spec and add condition to agent upgrade endpoint (#79579) [ML] Hide Data Grid column options when histogram charts are enabled. (#79459) [Telemetry] Synchronous `setup` and `start` methods (#79457) [Observability] Persist time range across apps (#79258) ...

lukeelmers force-pushed the test/data-bundle branch from 69fed12 to d003dfe Compare October 1, 2020 21:22

lukeelmers commented Oct 1, 2020

View reviewed changes

lukeelmers self-assigned this Oct 1, 2020

lukeelmers added release_note:skip Skip the PR/issue when compiling release notes technical debt Improvement of the software architecture and operational architecture v7.10.0 v8.0.0 labels Oct 1, 2020

lukeelmers force-pushed the test/data-bundle branch from d003dfe to 3ee8d4d Compare October 2, 2020 03:35

lukeelmers added the Team:AppArch label Oct 2, 2020

lukeelmers added the review label Oct 2, 2020

lukeelmers marked this pull request as ready for review October 2, 2020 03:38

lukeelmers requested a review from a team as a code owner October 2, 2020 03:38

lukeelmers requested a review from a team October 2, 2020 03:38

lukeelmers requested review from a team as code owners October 2, 2020 03:38

mattkime approved these changes Oct 2, 2020

View reviewed changes

FrankHassanabad approved these changes Oct 2, 2020

View reviewed changes

ThomThomson approved these changes Oct 2, 2020

View reviewed changes

lukeelmers force-pushed the test/data-bundle branch from 3ee8d4d to 9f98cd8 Compare October 2, 2020 22:52

lukeelmers added 6 commits October 5, 2020 15:11

Lazy load SearchBar.

d7aa32a

Lazy load IndexPatternSelect.

7b3b372

Lazy load ApplyFiltersPopover.

7afdc2c

Lazy load QueryStringInput.

0dd3af9

Lazy load ShardFailureModal.

e618fdd

Lazy load typeahead SuggestionsComponent.

4ed11d5

lukeelmers added 5 commits October 5, 2020 15:12

Lazy load FilterLabel.

6ff71e1

Fix problematic imports.

4fbb5de

Fix security solution test which relies on SearchBar.

c1cafcc

Update generated docs.

3b4f48a

Update maps functional test.

acda6d9

lukeelmers force-pushed the test/data-bundle branch from 9f98cd8 to acda6d9 Compare October 5, 2020 22:38

lukeelmers commented Oct 5, 2020

View reviewed changes

Merge branch 'master' into test/data-bundle

47525ce

lukeelmers requested a review from a team as a code owner October 6, 2020 02:51

nreese approved these changes Oct 6, 2020

View reviewed changes

lukeelmers merged commit 8ec5ce6 into elastic:master Oct 6, 2020

lukeelmers deleted the test/data-bundle branch October 6, 2020 13:24

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Oct 6, 2020

[data.ui] Lazy load UI components in data plugin. (elastic#78889)

308bd13

lukeelmers mentioned this pull request Oct 6, 2020

[7.x] [data.ui] Lazy load UI components in data plugin. (#78889) #79659

Merged

lukeelmers added a commit that referenced this pull request Oct 6, 2020

[data.ui] Lazy load UI components in data plugin. (#78889) (#79659)

ed66ca7

lukeelmers mentioned this pull request Oct 15, 2020

[data.ui] Fix flaky test & lazy loading rendering artifacts. #80612

Merged

lukeelmers mentioned this pull request Dec 2, 2020

[data] Reduce data bundle size #84788

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[data.ui] Lazy load UI components in data plugin. #78889

[data.ui] Lazy load UI components in data plugin. #78889

lukeelmers commented Sep 29, 2020 •

edited

Loading

lukeelmers Oct 1, 2020

lukeelmers Oct 1, 2020

lukeelmers Oct 1, 2020

lukeelmers Oct 1, 2020

lukeelmers Oct 1, 2020

elasticmachine commented Oct 2, 2020

lukeelmers commented Oct 2, 2020

mattkime left a comment

FrankHassanabad left a comment

ThomThomson left a comment •

edited

Loading

lukeelmers commented Oct 2, 2020

lukeelmers Oct 5, 2020

kibanamachine commented Oct 6, 2020

nreese left a comment

		import { phraseFilter } from '../../../../stubs';

		afterEach(cleanup);

		await waitFor(() => getByText(kqlQuery.query));
		await waitFor(() => getByText('KQL'));

		// Allow a range of hits to account for variances in browser window size.
		expect(parseInt(hits)).to.be.within(30, 40);

[data.ui] Lazy load UI components in data plugin. #78889

[data.ui] Lazy load UI components in data plugin. #78889

Conversation

lukeelmers commented Sep 29, 2020 • edited Loading

Before: data/public/ui ~307k

After: data/public/ui ~37k

lukeelmers Oct 1, 2020

Choose a reason for hiding this comment

lukeelmers Oct 1, 2020

Choose a reason for hiding this comment

lukeelmers Oct 1, 2020

Choose a reason for hiding this comment

lukeelmers Oct 1, 2020

Choose a reason for hiding this comment

lukeelmers Oct 1, 2020

Choose a reason for hiding this comment

elasticmachine commented Oct 2, 2020

lukeelmers commented Oct 2, 2020

mattkime left a comment

Choose a reason for hiding this comment

FrankHassanabad left a comment

Choose a reason for hiding this comment

ThomThomson left a comment • edited Loading

Choose a reason for hiding this comment

lukeelmers commented Oct 2, 2020

lukeelmers Oct 5, 2020

Choose a reason for hiding this comment

kibanamachine commented Oct 6, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

async chunks size

distributable file count

page load bundle size

History

nreese left a comment

Choose a reason for hiding this comment

lukeelmers commented Sep 29, 2020 •

edited

Loading

Before: `data/public/ui` ~307k

After: `data/public/ui` ~37k

ThomThomson left a comment •

edited

Loading