Remove dynamic mapped types from UiActions #87075

joshdover · 2020-12-30T19:21:43Z

Summary

This removes the pseudo type safety in the UiActions framework that leveraged declare module to reopen the TriggerContextMapping type. This pattern is not compatible with TS project references (#80508) and is not really type safe anyways. This PR removes this type and simplifies a lot of related code.

Reasons we should feel comfortable removing this:

It creates implicit circular dependencies in the type system
It leaks types across plugins that are in the same TS project
I didn't find a single instance where this type information was actually leveraged by consuming code to catch errors. It appears all calling code already knew the type of action it was calling into (as it should).
This pattern must be removed to migrate to TS project references and to the Bazel build tool, neither of which will support circular imports between plugins.

For maintainers

This was checked for breaking API changes and was labeled appropriately

joshdover · 2020-12-30T19:29:04Z

src/plugins/ui_actions/public/actions/create_action.ts

  return {
    getIconType: () => undefined,
    order: 0,
-    id: action.type,


I'm not sure why this wasn't failing before, but ActionDefintion currently has id marked as a required field, yet this function does provide a default fallback to the type field.

TS was complaining here that this line would be overridden by the ...action spread below which makes sense, so I removed this line. However, that also required me to add the id field to many call sites of this function. I didn't want to change the types, but I think the desired behavior is:

ActionDefinition should require either id OR type

If id is undefined, createAction should fallback to type. Is that correct?

However, that also required me to add the id field to many call sites of this function. I didn't want to change the types, but I think the desired behavior is: ActionDefinition should require either id OR type If id is undefined, createAction should fallback to type. Is that correct?

This seems correct, but to not add id in bunch of call sites, why you didn't make id optional for this function lit it was before?
See: https://github.com/elastic/kibana/pull/87075/files?file-filters%5B%5D=.ts&file-filters%5B%5D=.tsx#diff-4960a0b834fad6726535d59e6fc8b2febb31f0669916cbcfe9f19eec00478f46L26

in the old implementation this was valid:

createAction({ execute() {} })

since both id and type used to be optional.

Josh decided to make id required instead of introducing a union type ActionDefinitionWithId | ActionDefinitionWithType

joshdover · 2020-12-30T19:40:11Z

x-pack/plugins/ui_actions_enhanced/public/drilldowns/drilldown_definition.ts

+  ExecutionContext extends object = object,
+  FactoryContext extends BaseActionFactoryContext = BaseActionFactoryContext


I switched the order of ExecutionContext and FactoryContext since there were a few use cases where the default for ExecutionContext needed to be overridden but not FactoryContext

elasticmachine · 2020-12-30T19:51:49Z

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

flash1293

Kibana app stuff LGTM, left one small nit

x-pack/plugins/discover_enhanced/public/plugin.ts

streamich

Code changes LGTM.

darnautov

ML changes LGTM

Dosant · 2021-01-04T11:02:51Z

I understand that we have to do this to unblock typescript project refs effort, but I wouldn't agree with this statement:

I didn't find a single instance where this type information was actually leveraged by consuming code to catch errors. It appears all calling code already knew the type of action it was calling into (as it should).

Some example from top of my head where old system was useful:

1. getTrigger returns type safe triggers

https://github.com/elastic/kibana/blob/master/src/plugins/data/public/actions/select_range_action.ts#L50-L52

uiActions.getTrigger(APPLY_FILTER_TRIGGER).exec({
            filters,
            embeddable: context.embeddable,
            timeFieldName: context.data.timeFieldName,
});

The context shape passed into exec function is typed. In this pr, as I understood, it would only type check that shape extends object.

Same applies for getTriggerCompatibleActions

2. Drilldown definition listed a type safe list of supported triggers.

https://github.com/elastic/kibana/blob/master/x-pack/plugins/ui_actions_enhanced/public/drilldowns/drilldown_definition.ts#L163

Consumers who create their drilldowns had to specify supported triggers, we would caught on type check if they are trying to use not existing trigger. (e.g. typo or older trigger was removed or renamed)

Same for embeddable listing a list of triggers they could emit.

3. We enforced Drilldown's context to be a superset of its triggers' contexts

https://github.com/elastic/kibana/blob/master/x-pack/plugins/ui_actions_enhanced/public/drilldowns/drilldown_definition.ts#L38

Imagine a drilldown type supports both VALUE_CLICK_TRIGGER and RANGE_SELECT_TRIGGER, in such case before this pr we type checked that drilldown's context can handle VALUE_CLICK_TRIGGER_CONTEXT | RANGE_SELECT_TRIGGER_CONTEXT

Dosant · 2021-01-04T11:15:35Z

src/plugins/ui_actions/public/actions/create_action.ts

  return {
    getIconType: () => undefined,
    order: 0,
-    id: action.type,


However, that also required me to add the id field to many call sites of this function. I didn't want to change the types, but I think the desired behavior is: ActionDefinition should require either id OR type If id is undefined, createAction should fallback to type. Is that correct?

This seems correct, but to not add id in bunch of call sites, why you didn't make id optional for this function lit it was before?
See: https://github.com/elastic/kibana/pull/87075/files?file-filters%5B%5D=.ts&file-filters%5B%5D=.tsx#diff-4960a0b834fad6726535d59e6fc8b2febb31f0669916cbcfe9f19eec00478f46L26

src/plugins/ui_actions/public/service/ui_actions_service.test.ts

src/plugins/ui_actions/public/service/ui_actions_service.ts

src/plugins/ui_actions/public/tests/test_samples/hello_world_action.tsx

mshustov · 2021-01-04T13:50:35Z

@Dosant as @joshdover is on PTO, I'm going to continue working on the task.

getTrigger returns type safe triggers
https://github.com/elastic/kibana/blob/master/src/plugins/data/public/actions/select_range_action.ts#L50-L52
The context shape passed into exec function is typed. In this pr, as I understood, it would only type check that shape extends object.

If I understand correctly, the point was that introduced complexity doesn't justify the effort. Since we do know the type of the action, the type safety can be achieved as simple as

const triggerContext: ApplyGlobalFilterActionContext = {
  filters,
  embeddable: context.embeddable,
  timeFieldName: context.data.timeFieldName,
};
uiActions.getTrigger(APPLY_FILTER_TRIGGER).exec(triggerContext);

Drilldown definition listed a type safe list of supported triggers.
Consumers who create their drilldowns had to specify supported triggers, we would caught on type check if they are trying to use not existing trigger. (e.g. typo or older trigger was removed or renamed)

A consumer can achieve the same importing TriggerId from a plugin or plugin contract. With the current implementation, a plugin can call uiActions.getTrigger(TRIGGER_NAME).exec(..) without even declaring a dependency on a plugin registering TRIGGER_NAME (note that this plugin might be disabled as well, the type system won't provide any guarantees about that)

Any input on an alternative implementation is appreciated.

Dosant · 2021-01-04T14:28:34Z

@restrry,

Any input on an alternative implementation is appreciated.

Unfortunately, don't have any alternatives on my mind.
To be clear: with that note above I wasn't pushing back on this change. My intention was to note, that we have to lose some of benefits of older system (it isn't clear from the pr description).

would appreciate those nit code comments clean up.

Also. Does this mean we have to get rid of this pattern in other places? One other heavily used I am aware of is share plugin and Url generators.

mshustov · 2021-01-04T16:13:04Z

@Dosant answered nits.

Does this mean we have to get rid of this pattern in other places? One other heavily used I am aware of is share plugin and Url generators.

Probably, we definitely want to get rid of it in the core code #57588

clintandrewhall

Looks straight-forward to me. 👍

kibanamachine · 2021-01-04T18:23:38Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2b87332

Metrics [docs]

Distributable file count

id	before	after	diff
`default`	47266	48026	+760

Page load bundle

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

id	before	after	diff
`discoverEnhanced`	15.7KB	15.4KB	-339.0B
`embeddable`	233.0KB	233.0KB	+43.0B
`maps`	152.7KB	152.8KB	+51.0B
`uiActions`	63.6KB	62.3KB	-1.4KB
total			-1.6KB

History

💔 Build #96656 failed c0f69a4
💚 Build #96472 succeeded 4ebe41d
💚 Build #96460 succeeded fcd5f4deb96a8f62650b0e8006e398a8408e6b93

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

* Remove dynamic mapped types from UiActions * Remove import between data <-> embeddables * remove outdated comments, export action types from discover_enhanced * fix notice.txt Co-authored-by: restrry <[email protected]>

* Remove dynamic mapped types from UiActions * Remove import between data <-> embeddables * remove outdated comments, export action types from discover_enhanced * fix notice.txt Co-authored-by: restrry <[email protected]> Co-authored-by: Josh Dover <[email protected]>

joshdover commented Dec 30, 2020

View reviewed changes

joshdover added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 v8.0.0 Team:AppServices labels Dec 30, 2020

joshdover force-pushed the ui-actions-remove-types branch from fcd5f4d to a27d42e Compare December 30, 2020 19:51

joshdover marked this pull request as ready for review December 30, 2020 19:51

joshdover requested a review from a team December 30, 2020 19:51

joshdover requested review from a team as code owners December 30, 2020 19:51

joshdover requested a review from Dosant December 30, 2020 19:51

Remove dynamic mapped types from UiActions

be317b3

joshdover force-pushed the ui-actions-remove-types branch from a27d42e to be317b3 Compare December 30, 2020 19:55

botelastic bot added Feature:Drilldowns Embeddable panel Drilldowns Feature:Embedding Embedding content via iFrame labels Dec 30, 2020

Remove import between data <-> embeddables

4ebe41d

joshdover mentioned this pull request Dec 30, 2020

Migrate data plugin and dependencies to TS project references #87081

Closed

8 tasks

flash1293 approved these changes Jan 4, 2021

View reviewed changes

x-pack/plugins/discover_enhanced/public/plugin.ts Show resolved Hide resolved

streamich approved these changes Jan 4, 2021

View reviewed changes

darnautov approved these changes Jan 4, 2021

View reviewed changes

Dosant reviewed Jan 4, 2021

View reviewed changes

mshustov requested a review from Dosant January 4, 2021 13:52

remove outdated comments, export action types from discover_enhanced

c0f69a4

mshustov added 2 commits January 4, 2021 17:13

fix notice.txt

1f8778b

Merge branch 'master' into pr/87075

2b87332

clintandrewhall approved these changes Jan 4, 2021

View reviewed changes

Dosant approved these changes Jan 5, 2021

View reviewed changes

mshustov merged commit 0af8131 into elastic:master Jan 5, 2021

mshustov mentioned this pull request Jan 5, 2021

[7.x] Remove dynamic mapped types from UiActions (#87075) #87274

Merged

mshustov added the backported label Jan 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dynamic mapped types from UiActions #87075

Remove dynamic mapped types from UiActions #87075

joshdover commented Dec 30, 2020 •

edited

Loading

joshdover Dec 30, 2020

Dosant Jan 4, 2021

mshustov Jan 4, 2021

joshdover Dec 30, 2020

elasticmachine commented Dec 30, 2020

flash1293 left a comment

streamich left a comment

darnautov left a comment

Dosant commented Jan 4, 2021 •

edited

Loading

Dosant Jan 4, 2021

mshustov commented Jan 4, 2021 •

edited

Loading

Dosant commented Jan 4, 2021

mshustov commented Jan 4, 2021

clintandrewhall left a comment

kibanamachine commented Jan 4, 2021

		ExecutionContext extends object = object,
		FactoryContext extends BaseActionFactoryContext = BaseActionFactoryContext

Remove dynamic mapped types from UiActions #87075

Remove dynamic mapped types from UiActions #87075

Conversation

joshdover commented Dec 30, 2020 • edited Loading

Summary

For maintainers

joshdover Dec 30, 2020

Choose a reason for hiding this comment

Dosant Jan 4, 2021

Choose a reason for hiding this comment

mshustov Jan 4, 2021

Choose a reason for hiding this comment

joshdover Dec 30, 2020

Choose a reason for hiding this comment

elasticmachine commented Dec 30, 2020

flash1293 left a comment

Choose a reason for hiding this comment

streamich left a comment

Choose a reason for hiding this comment

darnautov left a comment

Choose a reason for hiding this comment

Dosant commented Jan 4, 2021 • edited Loading

1. getTrigger returns type safe triggers

2. Drilldown definition listed a type safe list of supported triggers.

3. We enforced Drilldown's context to be a superset of its triggers' contexts

Dosant Jan 4, 2021

Choose a reason for hiding this comment

mshustov commented Jan 4, 2021 • edited Loading

Dosant commented Jan 4, 2021

mshustov commented Jan 4, 2021

clintandrewhall left a comment

Choose a reason for hiding this comment

kibanamachine commented Jan 4, 2021

💚 Build Succeeded

Metrics [docs]

Distributable file count

Page load bundle

History

joshdover commented Dec 30, 2020 •

edited

Loading

Dosant commented Jan 4, 2021 •

edited

Loading

mshustov commented Jan 4, 2021 •

edited

Loading