Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Further improve type checking for actions and triggers #58765

Merged
merged 13 commits into from
Mar 4, 2020
Merged

Further improve type checking for actions and triggers #58765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
223a9d3
wip
stacey-gammon Feb 26, 2020
80f5761
review follow up
stacey-gammon Mar 2, 2020
dec2ceb
make ACTION a prefix, not SUFFIX
stacey-gammon Mar 2, 2020
4cffe80
fix path
stacey-gammon Mar 2, 2020
d58dcbb
Merge branch 'master' of github.com:elastic/kibana into 2020-02-26-ac…
stacey-gammon Mar 2, 2020
e950e1d
add warnings about casting to ActionType
stacey-gammon Mar 2, 2020
f97c9c3
Make context an object in examples, not a string
stacey-gammon Mar 2, 2020
5e50447
require object context, which seems to fix the partial requirement in…
stacey-gammon Mar 2, 2020
fb29e4e
mistake
stacey-gammon Mar 3, 2020
689dbc5
Merge branch 'master' into 2020-02-26-action-types-2
elasticmachine Mar 3, 2020
dcdec80
Merge branch 'master' of github.com:elastic/kibana into 2020-02-26-ac…
stacey-gammon Mar 3, 2020
4b29831
Merge branch 'master' of github.com:elastic/kibana into 2020-02-26-ac…
stacey-gammon Mar 3, 2020
40c51f8
Merge branch 'master' of github.com:elastic/kibana into 2020-02-26-ac…
stacey-gammon Mar 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/ui_action_examples/public/hello_world_action.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@ import { OverlayStart } from '../../../src/core/public';
import { createAction } from '../../../src/plugins/ui_actions/public';
import { toMountPoint } from '../../../src/plugins/kibana_react/public';

export const HELLO_WORLD_ACTION_TYPE = 'HELLO_WORLD_ACTION_TYPE';
export const ACTION_HELLO_WORLD = 'ACTION_HELLO_WORLD';

interface StartServices {
openModal: OverlayStart['openModal'];
}

export const createHelloWorldAction = (getStartServices: () => Promise<StartServices>) =>
createAction({
type: HELLO_WORLD_ACTION_TYPE,
type: ACTION_HELLO_WORLD,
getDisplayName: () => 'Hello World!',
execute: async () => {
const { openModal } = await getStartServices();
Expand Down
2 changes: 1 addition & 1 deletion examples/ui_action_examples/public/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,4 @@ import { PluginInitializer } from '../../../src/core/public';
export const plugin: PluginInitializer<void, void> = () => new UiActionExamplesPlugin();

export { HELLO_WORLD_TRIGGER_ID } from './hello_world_trigger';
export { HELLO_WORLD_ACTION_TYPE } from './hello_world_action';
export { ACTION_HELLO_WORLD } from './hello_world_action';
10 changes: 7 additions & 3 deletions examples/ui_action_examples/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import { Plugin, CoreSetup } from '../../../src/core/public';
import { UiActionsSetup } from '../../../src/plugins/ui_actions/public';
import { createHelloWorldAction } from './hello_world_action';
import { createHelloWorldAction, ACTION_HELLO_WORLD } from './hello_world_action';
import { helloWorldTrigger, HELLO_WORLD_TRIGGER_ID } from './hello_world_trigger';

interface UiActionExamplesSetupDependencies {
Expand All @@ -28,7 +28,11 @@ interface UiActionExamplesSetupDependencies {

declare module '../../../src/plugins/ui_actions/public' {
export interface TriggerContextMapping {
[HELLO_WORLD_TRIGGER_ID]: undefined;
[HELLO_WORLD_TRIGGER_ID]: {};
}

export interface ActionContextMapping {
[ACTION_HELLO_WORLD]: {};
}
}

Expand All @@ -42,7 +46,7 @@ export class UiActionExamplesPlugin
}));

uiActions.registerAction(helloWorldAction);
uiActions.attachAction(helloWorldTrigger.id, helloWorldAction.id);
uiActions.attachAction(helloWorldTrigger.id, helloWorldAction);
}

public start() {}
Expand Down
59 changes: 30 additions & 29 deletions examples/ui_actions_explorer/public/actions/actions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,44 +27,48 @@ export const USER_TRIGGER = 'USER_TRIGGER';
export const COUNTRY_TRIGGER = 'COUNTRY_TRIGGER';
export const PHONE_TRIGGER = 'PHONE_TRIGGER';

export const VIEW_IN_MAPS_ACTION = 'VIEW_IN_MAPS_ACTION';
export const TRAVEL_GUIDE_ACTION = 'TRAVEL_GUIDE_ACTION';
export const CALL_PHONE_NUMBER_ACTION = 'CALL_PHONE_NUMBER_ACTION';
export const EDIT_USER_ACTION = 'EDIT_USER_ACTION';
export const PHONE_USER_ACTION = 'PHONE_USER_ACTION';
export const SHOWCASE_PLUGGABILITY_ACTION = 'SHOWCASE_PLUGGABILITY_ACTION';
export const ACTION_VIEW_IN_MAPS = 'ACTION_VIEW_IN_MAPS';
export const ACTION_TRAVEL_GUIDE = 'ACTION_TRAVEL_GUIDE';
export const ACTION_CALL_PHONE_NUMBER = 'ACTION_CALL_PHONE_NUMBER';
export const ACTION_EDIT_USER = 'ACTION_EDIT_USER';
export const ACTION_PHONE_USER = 'ACTION_PHONE_USER';
export const ACTION_SHOWCASE_PLUGGABILITY = 'ACTION_SHOWCASE_PLUGGABILITY';

export const showcasePluggability = createAction({
type: SHOWCASE_PLUGGABILITY_ACTION,
export const showcasePluggability = createAction<typeof ACTION_SHOWCASE_PLUGGABILITY>({
type: ACTION_SHOWCASE_PLUGGABILITY,
getDisplayName: () => 'This is pluggable! Any plugin can inject their actions here.',
execute: async () => alert("Isn't that cool?!"),
});

export type PhoneContext = string;
export interface PhoneContext {
phone: string;
}

export const makePhoneCallAction = createAction<PhoneContext>({
type: CALL_PHONE_NUMBER_ACTION,
export const makePhoneCallAction = createAction<typeof ACTION_CALL_PHONE_NUMBER>({
type: ACTION_CALL_PHONE_NUMBER,
getDisplayName: () => 'Call phone number',
execute: async phone => alert(`Pretend calling ${phone}...`),
execute: async context => alert(`Pretend calling ${context.phone}...`),
});

export const lookUpWeatherAction = createAction<{ country: string }>({
type: TRAVEL_GUIDE_ACTION,
export const lookUpWeatherAction = createAction<typeof ACTION_TRAVEL_GUIDE>({
type: ACTION_TRAVEL_GUIDE,
getIconType: () => 'popout',
getDisplayName: () => 'View travel guide',
execute: async ({ country }) => {
window.open(`https://www.worldtravelguide.net/?s=${country},`, '_blank');
execute: async context => {
window.open(`https://www.worldtravelguide.net/?s=${context.country}`, '_blank');
},
});

export type CountryContext = string;
export interface CountryContext {
country: string;
}

export const viewInMapsAction = createAction<CountryContext>({
type: VIEW_IN_MAPS_ACTION,
export const viewInMapsAction = createAction<typeof ACTION_VIEW_IN_MAPS>({
type: ACTION_VIEW_IN_MAPS,
getIconType: () => 'popout',
getDisplayName: () => 'View in maps',
execute: async country => {
window.open(`https://www.google.com/maps/place/${country}`, '_blank');
execute: async context => {
window.open(`https://www.google.com/maps/place/${context.country}`, '_blank');
},
});

Expand Down Expand Up @@ -100,11 +104,8 @@ function EditUserModal({
}

export const createEditUserAction = (getOpenModal: () => Promise<OverlayStart['openModal']>) =>
createAction<{
user: User;
update: (user: User) => void;
}>({
type: EDIT_USER_ACTION,
createAction<typeof ACTION_EDIT_USER>({
type: ACTION_EDIT_USER,
getIconType: () => 'pencil',
getDisplayName: () => 'Edit user',
execute: async ({ user, update }) => {
Expand All @@ -120,8 +121,8 @@ export interface UserContext {
}

export const createPhoneUserAction = (getUiActionsApi: () => Promise<UiActionsStart>) =>
createAction<UserContext>({
type: PHONE_USER_ACTION,
createAction<typeof ACTION_PHONE_USER>({
type: ACTION_PHONE_USER,
getDisplayName: () => 'Call phone number',
isCompatible: async ({ user }) => user.phone !== undefined,
execute: async ({ user }) => {
Expand All @@ -133,7 +134,7 @@ export const createPhoneUserAction = (getUiActionsApi: () => Promise<UiActionsSt
// TODO: we need to figure out the best way to handle these nested actions however, since
// we don't want multiple context menu's to pop up.
if (user.phone !== undefined) {
(await getUiActionsApi()).executeTriggerActions(PHONE_TRIGGER, user.phone);
(await getUiActionsApi()).executeTriggerActions(PHONE_TRIGGER, { phone: user.phone });
}
},
});
11 changes: 6 additions & 5 deletions examples/ui_actions_explorer/public/app.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { EuiModalBody } from '@elastic/eui';
import { toMountPoint } from '../../../src/plugins/kibana_react/public';
import { UiActionsStart, createAction } from '../../../src/plugins/ui_actions/public';
import { AppMountParameters, OverlayStart } from '../../../src/core/public';
import { HELLO_WORLD_TRIGGER_ID, HELLO_WORLD_ACTION_TYPE } from '../../ui_action_examples/public';
import { HELLO_WORLD_TRIGGER_ID, ACTION_HELLO_WORLD } from '../../ui_action_examples/public';
import { TriggerContextExample } from './trigger_context_example';

interface Props {
Expand All @@ -60,7 +60,7 @@ const ActionsExplorer = ({ uiActionsApi, openModal }: Props) => {
</EuiText>
<EuiButton
data-test-subj="emitHelloWorldTrigger"
onClick={() => uiActionsApi.executeTriggerActions(HELLO_WORLD_TRIGGER_ID, undefined)}
onClick={() => uiActionsApi.executeTriggerActions(HELLO_WORLD_TRIGGER_ID, {})}
>
Say hello world!
</EuiButton>
Expand All @@ -76,8 +76,9 @@ const ActionsExplorer = ({ uiActionsApi, openModal }: Props) => {
<EuiButton
data-test-subj="addDynamicAction"
onClick={() => {
const dynamicAction = createAction<{}>({
type: `${HELLO_WORLD_ACTION_TYPE}-${name}`,
const dynamicAction = createAction<typeof ACTION_HELLO_WORLD>({
id: `${ACTION_HELLO_WORLD}-${name}`,
type: ACTION_HELLO_WORLD,
getDisplayName: () => `Say hello to ${name}`,
execute: async () => {
const overlay = openModal(
Expand All @@ -95,7 +96,7 @@ const ActionsExplorer = ({ uiActionsApi, openModal }: Props) => {
},
});
uiActionsApi.registerAction(dynamicAction);
uiActionsApi.attachAction(HELLO_WORLD_TRIGGER_ID, dynamicAction.type);
uiActionsApi.attachAction(HELLO_WORLD_TRIGGER_ID, dynamicAction);
setConfirmationText(
`You've successfully added a new action: ${dynamicAction.getDisplayName(
{}
Expand Down
48 changes: 26 additions & 22 deletions examples/ui_actions_explorer/public/plugin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,17 @@ import {
lookUpWeatherAction,
viewInMapsAction,
createEditUserAction,
CALL_PHONE_NUMBER_ACTION,
VIEW_IN_MAPS_ACTION,
TRAVEL_GUIDE_ACTION,
PHONE_USER_ACTION,
EDIT_USER_ACTION,
makePhoneCallAction,
showcasePluggability,
SHOWCASE_PLUGGABILITY_ACTION,
UserContext,
CountryContext,
PhoneContext,
ACTION_EDIT_USER,
ACTION_SHOWCASE_PLUGGABILITY,
ACTION_CALL_PHONE_NUMBER,
ACTION_TRAVEL_GUIDE,
ACTION_VIEW_IN_MAPS,
ACTION_PHONE_USER,
} from './actions/actions';

interface StartDeps {
Expand All @@ -54,6 +54,15 @@ declare module '../../../src/plugins/ui_actions/public' {
[COUNTRY_TRIGGER]: CountryContext;
[PHONE_TRIGGER]: PhoneContext;
}

export interface ActionContextMapping {
[ACTION_EDIT_USER]: UserContext;
[ACTION_SHOWCASE_PLUGGABILITY]: {};
[ACTION_CALL_PHONE_NUMBER]: PhoneContext;
[ACTION_TRAVEL_GUIDE]: CountryContext;
[ACTION_VIEW_IN_MAPS]: CountryContext;
[ACTION_PHONE_USER]: UserContext;
}
}

export class UiActionsExplorerPlugin implements Plugin<void, void, {}, StartDeps> {
Expand All @@ -67,29 +76,24 @@ export class UiActionsExplorerPlugin implements Plugin<void, void, {}, StartDeps
deps.uiActions.registerTrigger({
id: USER_TRIGGER,
});
deps.uiActions.registerAction(lookUpWeatherAction);
deps.uiActions.registerAction(viewInMapsAction);
deps.uiActions.registerAction(makePhoneCallAction);
deps.uiActions.registerAction(showcasePluggability);

const startServices = core.getStartServices();
deps.uiActions.registerAction(

deps.uiActions.attachAction(
USER_TRIGGER,
createPhoneUserAction(async () => (await startServices)[1].uiActions)
);
deps.uiActions.registerAction(
deps.uiActions.attachAction(
USER_TRIGGER,
createEditUserAction(async () => (await startServices)[0].overlays.openModal)
);
deps.uiActions.attachAction(USER_TRIGGER, PHONE_USER_ACTION);
deps.uiActions.attachAction(USER_TRIGGER, EDIT_USER_ACTION);

// What's missing here is type analysis to ensure the context emitted by the trigger
// is the same context that the action requires.
deps.uiActions.attachAction(COUNTRY_TRIGGER, VIEW_IN_MAPS_ACTION);
deps.uiActions.attachAction(COUNTRY_TRIGGER, TRAVEL_GUIDE_ACTION);
deps.uiActions.attachAction(COUNTRY_TRIGGER, SHOWCASE_PLUGGABILITY_ACTION);
deps.uiActions.attachAction(PHONE_TRIGGER, CALL_PHONE_NUMBER_ACTION);
deps.uiActions.attachAction(PHONE_TRIGGER, SHOWCASE_PLUGGABILITY_ACTION);
deps.uiActions.attachAction(USER_TRIGGER, SHOWCASE_PLUGGABILITY_ACTION);
deps.uiActions.attachAction(COUNTRY_TRIGGER, viewInMapsAction);
Copy link
Contributor

@Dosant Dosant Mar 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I tried to replace viewInMaps for showcasePluggability and typescript didn't complain.
I run node scripts/type_check --project examples/ui_actions_explorer/tsconfig.json to make sure it is not my IDE problem

I am not sure if this is expected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is expected. showcasePluggability doesn't require any context so it can be attached to any trigger. It's okay to attach an action to a trigger that emits more than the action requires.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this makes sense, thanks.

How about this example, I find it particularly weird:

I change it to:

deps.uiActions.attachAction(USER_TRIGGER, lookUpWeatherAction);

And see the ts error message. "can't assign string to Partial | undefined" which is great.

But then I went and changed UserContext from type Country = string; to type Country = {country: string}

And this no longer throws typescript error for me 🤔

deps.uiActions.attachAction(USER_TRIGGER, lookUpWeatherAction);

It seems that it should complain in this case. Not sure if this is me or the current types are not working well for matching between different object shapes because of Partial.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome catch! It was an issue with Partial. I'm honestly not sure why it works now, but it seems like requiring an object let me get rid of partial type and now it works. Strange, but when I tested, (I changed the trigger context shape to CountryContext & { extra: string }) it seems to work now:

Screen Shot 2020-03-02 at 4 27 50 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! seems like working as expected 👍

deps.uiActions.attachAction(COUNTRY_TRIGGER, lookUpWeatherAction);
deps.uiActions.attachAction(COUNTRY_TRIGGER, showcasePluggability);
deps.uiActions.attachAction(PHONE_TRIGGER, makePhoneCallAction);
deps.uiActions.attachAction(PHONE_TRIGGER, showcasePluggability);
deps.uiActions.attachAction(USER_TRIGGER, showcasePluggability);

core.application.register({
id: 'uiActionsExplorer',
Expand Down
4 changes: 2 additions & 2 deletions examples/ui_actions_explorer/public/trigger_context_example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ const createRowData = (
<Fragment>
<EuiButtonEmpty
onClick={() => {
uiActionsApi.executeTriggerActions(COUNTRY_TRIGGER, user.countryOfResidence);
uiActionsApi.executeTriggerActions(COUNTRY_TRIGGER, { country: user.countryOfResidence });
}}
>
{user.countryOfResidence}
Expand All @@ -59,7 +59,7 @@ const createRowData = (
<EuiButtonEmpty
disabled={user.phone === undefined}
onClick={() => {
uiActionsApi.executeTriggerActions(PHONE_TRIGGER, user.phone!);
uiActionsApi.executeTriggerActions(PHONE_TRIGGER, { phone: user.phone! });
}}
>
{user.phone}
Expand Down
18 changes: 9 additions & 9 deletions src/legacy/core_plugins/data/public/actions/select_range_action.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,21 @@

import { i18n } from '@kbn/i18n';
import {
Action,
createAction,
IncompatibleActionError,
ActionByType,
} from '../../../../../plugins/ui_actions/public';
import { onBrushEvent } from './filters/brush_event';
import { FilterManager, TimefilterContract, esFilters } from '../../../../../plugins/data/public';

export const SELECT_RANGE_ACTION = 'SELECT_RANGE_ACTION';
export const ACTION_SELECT_RANGE = 'ACTION_SELECT_RANGE';

interface ActionContext {
export interface SelectRangeActionContext {
data: any;
timeFieldName: string;
}

async function isCompatible(context: ActionContext) {
async function isCompatible(context: SelectRangeActionContext) {
try {
return Boolean(await onBrushEvent(context.data));
} catch {
Expand All @@ -44,17 +44,17 @@ async function isCompatible(context: ActionContext) {
export function selectRangeAction(
filterManager: FilterManager,
timeFilter: TimefilterContract
): Action<ActionContext> {
return createAction<ActionContext>({
type: SELECT_RANGE_ACTION,
id: SELECT_RANGE_ACTION,
): ActionByType<typeof ACTION_SELECT_RANGE> {
return createAction<typeof ACTION_SELECT_RANGE>({
type: ACTION_SELECT_RANGE,
id: ACTION_SELECT_RANGE,
getDisplayName: () => {
return i18n.translate('data.filter.applyFilterActionTitle', {
defaultMessage: 'Apply filter to current view',
});
},
isCompatible,
execute: async ({ timeFieldName, data }: ActionContext) => {
execute: async ({ timeFieldName, data }: SelectRangeActionContext) => {
if (!(await isCompatible({ timeFieldName, data }))) {
throw new IncompatibleActionError();
}
Expand Down
18 changes: 9 additions & 9 deletions src/legacy/core_plugins/data/public/actions/value_click_action.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import { i18n } from '@kbn/i18n';
import { toMountPoint } from '../../../../../plugins/kibana_react/public';
import {
Action,
ActionByType,
createAction,
IncompatibleActionError,
} from '../../../../../plugins/ui_actions/public';
Expand All @@ -37,14 +37,14 @@ import {
esFilters,
} from '../../../../../plugins/data/public';

export const VALUE_CLICK_ACTION = 'VALUE_CLICK_ACTION';
export const ACTION_VALUE_CLICK = 'ACTION_VALUE_CLICK';

interface ActionContext {
export interface ValueClickActionContext {
data: any;
timeFieldName: string;
}

async function isCompatible(context: ActionContext) {
async function isCompatible(context: ValueClickActionContext) {
try {
const filters: Filter[] =
(await createFiltersFromEvent(context.data.data || [context.data], context.data.negate)) ||
Expand All @@ -58,17 +58,17 @@ async function isCompatible(context: ActionContext) {
export function valueClickAction(
filterManager: FilterManager,
timeFilter: TimefilterContract
): Action<ActionContext> {
return createAction<ActionContext>({
type: VALUE_CLICK_ACTION,
id: VALUE_CLICK_ACTION,
): ActionByType<typeof ACTION_VALUE_CLICK> {
return createAction<typeof ACTION_VALUE_CLICK>({
type: ACTION_VALUE_CLICK,
id: ACTION_VALUE_CLICK,
getDisplayName: () => {
return i18n.translate('data.filter.applyFilterActionTitle', {
defaultMessage: 'Apply filter to current view',
});
},
isCompatible,
execute: async ({ timeFieldName, data }: ActionContext) => {
execute: async ({ timeFieldName, data }: ValueClickActionContext) => {
if (!(await isCompatible({ timeFieldName, data }))) {
throw new IncompatibleActionError();
}
Expand Down
Loading