lens should register expression functions in setup contract #110639

alexwizp · 2021-08-31T14:41:48Z

Summary

lens expression functions are currently registered too late. they should be registered in setup life cycle to enable #106509

as functions are registered too late this will prevent us to run migrations on them, do reference extraction/injection or in the future use alerting or reporting services.

Closes: elastic#106510

dej611 · 2021-09-03T14:47:20Z

x-pack/plugins/lens/public/embeddable/embeddable_factory.ts

@@ -21,7 +21,7 @@ import { UiActionsStart } from '../../../../../src/plugins/ui_actions/public';
 import { Start as InspectorStart } from '../../../../../src/plugins/inspector/public';
 import { Document } from '../persistence/saved_object_store';
 import { LensAttributeService } from '../lens_attribute_service';
-import { DOC_TYPE } from '../../common';
+import { DOC_TYPE } from '../../common/constants';


why this change?

The main problem with export * is, It cause excessive exports in required bundle see #110891. On the other hand if we are talking about build package it's important to don't load extra modules. Yes, I agree that it probably doesn't look professional, but it allow us optimize bundle size a little bit.

I'm fine with having specific export directives, but in this case I'm unsure about the real gain here.

Do we have any measurable difference between import { DOC_TYPE } from '../common' and import {DOC_TYPE} from '../common/constants'? The item is exported via common/index which is pretty much imported fully imported in the setup phase of Lens.
Isn't treeshaking already taking care of all these esm exporting implementation details?

maybe if we avoid of using export * it starts to work correctly, but I found extra code in final bundle from which we used only constants. Replacing to '../common/constants' solved that issue

During the sync today Alexey demonstrated that changing this import gives us 4 KB of a smaller bundle. We agreed the change is worth it :)

# Conflicts: # x-pack/plugins/lens/public/lens_attribute_service.ts # x-pack/plugins/lens/public/plugin.ts

alexwizp · 2021-09-06T07:50:08Z

@elasticmachine merge upstream

elasticmachine · 2021-09-06T09:56:17Z

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

x-pack/plugins/lens/common/expressions/time_scale/types.ts

Co-authored-by: Marta Bondyra <[email protected]>

mbondyra

Great work, thank you for taking care of this one 🙇🏼‍♀️ I went through the code and didn't find anything suspicious, but it'd be good if @ppisljar and @dej611 take a look too. Tested on Chrome.

dej611

Code review LGTM

kibanamachine · 2021-09-06T13:25:22Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	829	834	+5

Async chunks

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

id	before	after	diff
`lens`	1.6MB	1.6MB	-53.6KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`lens`	30.7KB	58.6KB	+27.9KB

Unknown metric groups

async chunk count

id	before	after	diff
`lens`	3	9	+6

History

💔 Build #151240 failed 511c129
💛 Build #151183 was flaky 16b1b6d
💚 Build #151020 succeeded 2dc09df
💚 Build #150971 succeeded cb3dac9
💔 Build #150903 failed 9417d34

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

cc @alexwizp

ppisljar · 2021-09-06T13:40:37Z

x-pack/plugins/lens/public/expressions.ts

+    heatmapGridConfig,
+    axisExtentConfig,
+    labelsOrientationConfig,
+    getDatatable(formatFactory),


all functions should be registered on the server side as well (not necessary part of this PR)

ppisljar

code LGTM

…110639) * lens should register expression functions in setup contract Closes: elastic#106510 * fix CI * build optimization * build optimizations - step 3 * fix CI * try to optimize bundle * Update x-pack/plugins/lens/common/expressions/time_scale/types.ts Co-authored-by: Marta Bondyra <[email protected]> * Update types.ts Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Marta Bondyra <[email protected]>

…#111324) * lens should register expression functions in setup contract Closes: #106510 * fix CI * build optimization * build optimizations - step 3 * fix CI * try to optimize bundle * Update x-pack/plugins/lens/common/expressions/time_scale/types.ts Co-authored-by: Marta Bondyra <[email protected]> * Update types.ts Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Marta Bondyra <[email protected]> Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Marta Bondyra <[email protected]>

alexwizp force-pushed the 106510 branch from aa50829 to 34e34ce Compare August 31, 2021 15:01

lens should register expression functions in setup contract

441e18d

Closes: elastic#106510

alexwizp force-pushed the 106510 branch from 34e34ce to 441e18d Compare August 31, 2021 15:31

alexwizp added 2 commits August 31, 2021 21:04

fix CI

c7cd802

Merge remote-tracking branch 'upstream/master' into 106510

ca072c7

alexwizp requested review from dej611 and mbondyra September 1, 2021 14:25

build optimization

fb2e83a

alexwizp force-pushed the 106510 branch from e0b6a97 to fb2e83a Compare September 2, 2021 20:05

alexwizp added 4 commits September 3, 2021 14:28

build optimizations - step 3

b0f81ba

Merge remote-tracking branch 'upstream/main' into 106510

9417d34

fix CI

fdd5e4c

try to optimize bundle

cb3dac9

dej611 reviewed Sep 3, 2021

View reviewed changes

Merge branch 'master' into 106510

2dc09df

# Conflicts: # x-pack/plugins/lens/public/lens_attribute_service.ts # x-pack/plugins/lens/public/plugin.ts

Merge branch 'master' into 106510

16b1b6d

alexwizp marked this pull request as ready for review September 6, 2021 08:39

alexwizp requested a review from a team as a code owner September 6, 2021 08:39

alexwizp self-assigned this Sep 6, 2021

alexwizp added Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.16.0 v8.0.0 labels Sep 6, 2021

alexwizp added the release_note:skip Skip the PR/issue when compiling release notes label Sep 6, 2021

mbondyra requested a review from a team September 6, 2021 10:21

mbondyra reviewed Sep 6, 2021

View reviewed changes

x-pack/plugins/lens/common/expressions/time_scale/types.ts Outdated Show resolved Hide resolved

Update x-pack/plugins/lens/common/expressions/time_scale/types.ts

511c129

Co-authored-by: Marta Bondyra <[email protected]>

Update types.ts

0eed8f9

mbondyra approved these changes Sep 6, 2021

View reviewed changes

dej611 approved these changes Sep 6, 2021

View reviewed changes

ppisljar reviewed Sep 6, 2021

View reviewed changes

ppisljar approved these changes Sep 6, 2021

View reviewed changes

alexwizp merged commit e2c6a03 into elastic:master Sep 7, 2021

alexwizp mentioned this pull request Sep 7, 2021

[7.x] lens should register expression functions in setup contract (#110639) #111324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lens should register expression functions in setup contract #110639

lens should register expression functions in setup contract #110639

alexwizp commented Aug 31, 2021 •

edited

Loading

dej611 Sep 3, 2021

alexwizp Sep 3, 2021

dej611 Sep 3, 2021

alexwizp Sep 3, 2021

mbondyra Sep 6, 2021

alexwizp commented Sep 6, 2021

elasticmachine commented Sep 6, 2021

mbondyra left a comment •

edited

Loading

dej611 left a comment

kibanamachine commented Sep 6, 2021

async chunk count

ppisljar Sep 6, 2021

ppisljar left a comment

lens should register expression functions in setup contract #110639

lens should register expression functions in setup contract #110639

Conversation

alexwizp commented Aug 31, 2021 • edited Loading

Summary

dej611 Sep 3, 2021

Choose a reason for hiding this comment

alexwizp Sep 3, 2021

Choose a reason for hiding this comment

dej611 Sep 3, 2021

Choose a reason for hiding this comment

alexwizp Sep 3, 2021

Choose a reason for hiding this comment

mbondyra Sep 6, 2021

Choose a reason for hiding this comment

alexwizp commented Sep 6, 2021

elasticmachine commented Sep 6, 2021

mbondyra left a comment • edited Loading

Choose a reason for hiding this comment

dej611 left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 6, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

ppisljar Sep 6, 2021

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

alexwizp commented Aug 31, 2021 •

edited

Loading

mbondyra left a comment •

edited

Loading