App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. #82791

lukeelmers · 2020-11-05T23:38:12Z

Closes #82047

As part of the work to migrate to TS project references in preparation for the new build toolchain, we needed to remove some circular dependencies that exist between the ui_actions and expressions / data plugins.

The main issue were two imports inside ui_actions which were used in the range select, value click, and apply filter triggers:

kibana/src/plugins/ui_actions/public/types.ts

Lines 30 to 31 in 2c1bc50

    
           import type { RangeSelectContext, ValueClickContext } from '../../embeddable/public'; 
        
           import type { ApplyGlobalFilterActionContext } from '../../data/public';

These were imported to include in the shared TriggerContextMapping, and the respective triggers for each were all registered from the ui_actions plugin itself. This PR splits everything up as follows:

apply filters trigger moved to data plugin & registered from there
value click, range select triggers moved to embeddable plugin & registered from there
TriggerContextMapping was augmented via declare module from embeddable and data
- this is a pattern we ultimately want to move away from; I've created an issue for that here: [ui_actions] Remove declare module pattern for trigger/action mappings #82790
downstream imports in dependent apps were updated accordingly
- fortunately all of the apps depending on these imports from ui_actions already had existing dependencies on embeddable and data, so this doesn't affect any existing dependencies of those plugins.
data dependencies (query/filter/timerange) were removed from embeddable
- They were used in EmbeddableInput types, but embeddable didn't really need to know about these
- Downstream apps instead could extend from EmbeddableInput to provide them, so that the dependency on data is made explicit
data dependencies on embeddable were removed
- This was trickier and involved duplicating a few data-specific pieces of embeddable-owned interfaces
- In general the typings for some of these ui actions & triggers are very difficult to separate cleanly because they combine embeddable & data-owned items

I did some comparison on this PR using the new find_plugins_with_circular_deps script that was introduced in #82867, and we've gone from 117 circular dependencies down to 28 🎊

Once this PR merges, all app services-owned circular dependencies should be addressed.

lukeelmers · 2020-11-13T16:47:39Z

In working on this, I realized that we also have a data <-> embeddables circular dependency with types.

Data relies on embeddable for the action contexts for value click & select range actions. Apply filter action also needs IEmbeddable to include in its context.

On the embeddable, side, data types are used for Filter Query and TimeRange.

This design is generally just confusing, particularly the fact that these data-owned action contexts need to know about embeddables. So far there are two ways I've thought of to mitigate this:

Define the types for the action context in embeddable (since it already has type dependencies on data), and keep the actions themselves in data. The way the global mapping magically works means that the actions which are in data shouldn't need to import the types from embeddable directly.
Edit: I actually don't think this will work as presumably you'd need a plugin to have refs to explicitly depend on another plugin, otherwise the declare module wouldn't be included
Move the actions entirely to embeddable, which would require embeddable importing some stateful utilities from data which are currently internal.
Refactor embeddables so they don't need to know about Filter/Query/TimeRange, similar to what we did recently with expressions. Then data would still have a type dependency on embeddables. I have no idea how difficult this would be, but it feels non-trivial.

None of these options feels great... overall I don't know enough about uiActions to understand why embeddables need to be included in some of these action contexts in the first place; they don't appear to be using embeddables for anything internally, just passing them through. This feels like a design smell that we should revisit, but that's outside the scope of this PR.

Open to any other ideas I may have overlooked here... But so far my ideas boil down to the tradeoff between whether data should depend on embeddables, or embeddables on data.

lukeelmers · 2020-11-16T20:36:52Z

Refactor embeddables so they don't need to know about Filter/Query/TimeRange, similar to what we did recently with expressions. Then data would still have a type dependency on embeddables. I have no idea how difficult this would be, but it feels non-trivial.

@ppisljar and i discussed this today... we both agreed that this is the preferable option, so i'm going to look into the level of effort here and go this direction if it's feasible.

joshdover · 2020-12-01T17:07:32Z

@lukeelmers Any progress or update here on how much effort this will be? Anything we can do to help with brainstorming solutions? The data plugin is currently blocking many teams from migrating to TS project references, which in turn will be blocking our migration to the Bazel build system.

lukeelmers · 2020-12-01T17:40:00Z

@joshdover At this point I think we have a clear path forward, but I had to pause to complete a few other high-priority items that need to make it into 7.11. I plan to pick this back up, but this probably won't be until late this week or early next.

I know we're holding up other folks, so this is still next on my priority list after our remaining 7.11 commitments are addressed.

joshdover · 2020-12-01T19:15:29Z

Sounds great 😄

lukeelmers · 2020-12-10T22:48:03Z

I ran node scripts/find_plugins_with_circular_deps --debug on this PR and on master to compare. These latest changes bring us from 117 failures down to 35 🎊

However, it created a new circular dependency data -> embeddable -> saved_objects -> data that we need to sort out what to do with.

The only option that isn't a significant amount of work is trying to get rid of the data dependency on embeddable (it only needs types). embeddable has a runtime dependency on saved_objects, and saved_objects has one on data, so there isn't really a simple way to remove either of those links without refactoring some plugin contracts.

Otherwise I've covered all circular deps in app services-related plugins.

lukeelmers · 2020-12-10T23:55:07Z

Okay, I think we're all good now! 🤞

Down to 28 circular deps from 117, and all items owned by app services should be addressed!

I updated the allowList for the circular deps check script so that no regressions are introduced.

lukeelmers · 2020-12-11T00:01:56Z

src/dev/run_find_plugins_with_circular_deps.ts

@@ -31,13 +31,9 @@ interface Options {
 type CircularDepList = Set<string>;

 const allowedList: CircularDepList = new Set([
+  'src/plugins/charts -> src/plugins/discover',


Removing the app services-owned circular deps revealed a new circular dep between app-owned plugins, so I added it to the allowList.

lukeelmers · 2020-12-11T00:05:12Z

src/plugins/data/public/actions/filters/create_filters_from_range_select.ts

 import { AggConfigSerialized } from '../../../common/search/aggs';

-export async function createFiltersFromRangeSelectAction(event: RangeSelectContext['data']) {


Rather than selecting the data property from RangeSelectContext, I've copied those pieces over here.

lukeelmers · 2020-12-11T00:05:24Z

src/plugins/data/public/actions/filters/create_filters_from_value_click.test.ts


 const mockField = {
  name: 'bytes',
  filterable: true,
 };

 describe('createFiltersFromValueClick', () => {
-  let dataPoints: ValueClickContext['data']['data'];


Rather than selecting the data property from ValueClickContext, I've copied those pieces over here.

lukeelmers · 2020-12-11T00:07:01Z

src/plugins/data/public/plugin.ts

    uiActions.registerAction(
      createFilterAction(queryService.filterManager, queryService.timefilter.timefilter)
    );

    uiActions.addTriggerAction(
-      SELECT_RANGE_TRIGGER,
+      'SELECT_RANGE_TRIGGER',


There wasn't a great way to keep these trigger IDs as consts since they lived in embeddable, so I just used the ID string here.

lukeelmers · 2020-12-11T00:08:13Z

src/plugins/embeddable/public/lib/test_samples/embeddables/filterable_embeddable.tsx

+export interface MockFilter {
+  $state?: any;
+  meta: any;
+  query?: any;
+}


It doesn't make sense for embeddables to know about filter in the first place, but since all of these test helpers existed, I went ahead and made a "fake" filter interface for use in the tests.

lukeelmers · 2020-12-11T00:09:09Z

src/plugins/expressions/common/expression_renderers/types.ts

+   * but expressions cannot know about vis or it creates a mess of circular dependencies.
+   * Downstream consumers of the uiState handler will need to cast for now.
+   */
+  uiState?: unknown;


We need to get rid of uiState in expressions eventually anyway, so this is just another reason to do so 😉

elasticmachine · 2020-12-11T00:10:30Z

Pinging @elastic/kibana-app-services (Team:AppServices)

stratoula

Great PR Luke! Thank you 🙂 KibanaApp code review LGTM. I did a small test locally and it works fine.

@alexwizp when this PR will be merged we should also adjust the changes on our PR that moves PersistedState to kibana-utils

mattkime

Code changes look good, took a look at dashboards and everything seems to be working well.

kibanamachine · 2020-12-19T01:13:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 548a0c6

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`data`	609	611	+2
`uiActions`	30	27	-3
total			-1

Async chunks

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

id	before	after	diff
`discover`	451.4KB	451.4KB	-51.0B
`maps`	2.8MB	2.8MB	-80.0B
`visTypeVislib`	643.6KB	643.6KB	-49.0B
`visualizations`	61.4KB	61.4KB	+1.0B
total			-179.0B

Distributable file count

id	before	after	diff
`default`	47297	48057	+760

Page load bundle

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

id	before	after	diff
`dashboardEnhanced`	34.8KB	34.5KB	-284.0B
`data`	1002.1KB	1002.5KB	+465.0B
`discoverEnhanced`	16.0KB	15.7KB	-281.0B
`embeddable`	228.8KB	233.8KB	+4.9KB
`uiActions`	69.4KB	63.6KB	-5.7KB
`urlDrilldown`	51.2KB	51.2KB	+26.0B
`visualizations`	169.2KB	169.2KB	-25.0B
total			-924.0B

History

💔 Build #95559 failed cc7691f
💔 Build #95572 failed 5088150
💔 Build #95069 failed 1f2368e
💔 Build #93735 failed 141acd1
💔 Build #93717 failed 92f6ff9

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

…le circular dependencies. (#82791) (#86608) * Move applyFilter, selectRange, valueClick triggers to data/embeddables. * Update imports. * Remove embeddable references to non-existent data plugin dependency. * remove data mocks from embeddable * Remove query, filters, timeRange from EmbeddableInput and move to apps. * Remove data plugin imports from embeddable test samples. * Remove circular dependencies caused by expressions renderer handlers. * Update circular deps allowList. * Remove data dependency on embeddable. * Revert accidental data plugin change. * Fix new circular deps issues. * Update generated docs. * Fix type errors in vis_type_xy * Fix inspector data table.

* master: (48 commits) Fix request with disabled aggregation (elastic#85696) [Security Solution][Detections][Threshold Rules] Threshold Rule Bug Fixes (elastic#84918) Removed a possibility to define two different names for Alert types on API and UI level. (elastic#86236) Bump Node.js from version 14.15.2 to 14.15.3 (elastic#86593) [index patterns] Fleep app - Keep saved object field list until field caps provides fields (elastic#85370) [Security Solutions] fix timeline tabs + layout (elastic#86581) Upgrade to hapi version 20 (elastic#85406) App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. (elastic#82791) Rename chartLibrary setting to legacyChartsLibrary (elastic#86529) [CI] TeamCity updates (elastic#85843) [Maps] Use Json for mvt-tests (elastic#86492) [Rollup Jobs] Added autofocus to cron editor (elastic#86324) [Monitoring][Alerting] CCR read exceptions alert (elastic#85908) [CI] Bump memory for main CI workers (elastic#86541) Explicitly set Elasticsearch heap size during CI and local development (elastic#86513) [App Search] Updates to results on the documents view (elastic#86181) [Discover] Change default sort handling (elastic#85561) [App Search] Convert DocumentCreationModal to DocumentCreationFlyout (elastic#86508) [App Search] Sample Engines should have access to the Crawler (elastic#86502) Fixed duplication of create new modal (elastic#86489) ...

lukeelmers added v8.0.0 Team:AppArch release_note:skip Skip the PR/issue when compiling release notes Feature:UIActions UI actions. These are client side only, not related to the server side actions.. v7.11.0 NeededFor:Core labels Nov 5, 2020

lukeelmers self-assigned this Nov 5, 2020

lukeelmers force-pushed the fix/uiactions-circular-deps branch 2 times, most recently from c50fcf9 to e531149 Compare November 6, 2020 00:20

lukeelmers force-pushed the fix/uiactions-circular-deps branch from e531149 to 92f6ff9 Compare December 10, 2020 22:34

lukeelmers changed the title ~~Remove uiActions <--> expressions/data circular dependencies.~~ App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. Dec 10, 2020

lukeelmers added Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) labels Dec 11, 2020

lukeelmers requested a review from ppisljar December 11, 2020 00:00

lukeelmers commented Dec 11, 2020

View reviewed changes

lukeelmers marked this pull request as ready for review December 11, 2020 00:10

lukeelmers requested a review from a team December 11, 2020 00:10

lukeelmers requested review from a team as code owners December 11, 2020 00:10

This was referenced Dec 11, 2020

Remove circular dependency between uiActions & data/embeddable plugins #82047

Closed

[Meta] Migrate Kibana plugins to TS project references #80508

Closed

lukeelmers force-pushed the fix/uiactions-circular-deps branch from 141acd1 to 1f2368e Compare December 17, 2020 03:58

tylersmalley approved these changes Dec 17, 2020

View reviewed changes

stratoula approved these changes Dec 17, 2020

View reviewed changes

alexwizp approved these changes Dec 17, 2020

View reviewed changes

stratoula mentioned this pull request Dec 17, 2020

Create vis_type_xy plugin to replace histogram, area and line charts #78154

Merged

6 tasks

mattkime approved these changes Dec 18, 2020

View reviewed changes

lukeelmers added 12 commits December 18, 2020 09:24

Move applyFilter, selectRange, valueClick triggers to data/embeddables.

becd589

Update imports.

9abb53a

Remove embeddable references to non-existent data plugin dependency.

45d5410

remove data mocks from embeddable

84f3b22

Remove query, filters, timeRange from EmbeddableInput and move to apps.

efd9ee0

Remove data plugin imports from embeddable test samples.

9855d00

Remove circular dependencies caused by expressions renderer handlers.

167fe53

Update circular deps allowList.

a1b956e

Remove data dependency on embeddable.

57e7808

Revert accidental data plugin change.

da443f1

Fix new circular deps issues.

9b4bf16

Update generated docs.

cc7691f

lukeelmers force-pushed the fix/uiactions-circular-deps branch from 1f2368e to cc7691f Compare December 18, 2020 21:52

lukeelmers added 2 commits December 18, 2020 15:30

Fix type errors in vis_type_xy

8546be3

Fix inspector data table.

548a0c6

lukeelmers force-pushed the fix/uiactions-circular-deps branch from 5088150 to 548a0c6 Compare December 18, 2020 22:58

lukeelmers merged commit 9a3e291 into elastic:master Dec 19, 2020

lukeelmers deleted the fix/uiactions-circular-deps branch December 19, 2020 01:34

lukeelmers mentioned this pull request Dec 20, 2020

[7.x] App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. (#82791) #86608

Merged

stratoula mentioned this pull request Dec 22, 2020

Remove circular deps between KibanaApp team plugins #84750

Closed

5 tasks

Dosant mentioned this pull request Feb 10, 2021

[UIActions] Embeddable and data plugin triggers/actions race condition #90929

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. #82791

App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. #82791

lukeelmers commented Nov 5, 2020 •

edited

Loading

lukeelmers commented Nov 13, 2020

lukeelmers commented Nov 16, 2020

joshdover commented Dec 1, 2020

lukeelmers commented Dec 1, 2020

joshdover commented Dec 1, 2020

lukeelmers commented Dec 10, 2020

lukeelmers commented Dec 10, 2020

lukeelmers Dec 11, 2020

lukeelmers Dec 11, 2020

lukeelmers Dec 11, 2020

lukeelmers Dec 11, 2020

lukeelmers Dec 11, 2020

lukeelmers Dec 11, 2020

elasticmachine commented Dec 11, 2020

stratoula left a comment

mattkime left a comment

kibanamachine commented Dec 19, 2020

	import type { RangeSelectContext, ValueClickContext } from '../../embeddable/public';
	import type { ApplyGlobalFilterActionContext } from '../../data/public';

		import { AggConfigSerialized } from '../../../common/search/aggs';

		export async function createFiltersFromRangeSelectAction(event: RangeSelectContext['data']) {

App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. #82791

App Services: Remove remaining uiActions, expressions, data, embeddable circular dependencies. #82791

Conversation

lukeelmers commented Nov 5, 2020 • edited Loading

lukeelmers commented Nov 13, 2020

lukeelmers commented Nov 16, 2020

joshdover commented Dec 1, 2020

lukeelmers commented Dec 1, 2020

joshdover commented Dec 1, 2020

lukeelmers commented Dec 10, 2020

lukeelmers commented Dec 10, 2020

lukeelmers Dec 11, 2020

Choose a reason for hiding this comment

lukeelmers Dec 11, 2020

Choose a reason for hiding this comment

lukeelmers Dec 11, 2020

Choose a reason for hiding this comment

lukeelmers Dec 11, 2020

Choose a reason for hiding this comment

lukeelmers Dec 11, 2020

Choose a reason for hiding this comment

lukeelmers Dec 11, 2020

Choose a reason for hiding this comment

elasticmachine commented Dec 11, 2020

stratoula left a comment

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 19, 2020

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Distributable file count

Page load bundle

History

lukeelmers commented Nov 5, 2020 •

edited

Loading