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

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 1 commit
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
8 changes: 6 additions & 2 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, HELLO_WORLD_ACTION_TYPE } from './hello_world_action';
import { helloWorldTrigger, HELLO_WORLD_TRIGGER_ID } from './hello_world_trigger';

interface UiActionExamplesSetupDependencies {
Expand All @@ -30,6 +30,10 @@ declare module '../../../src/plugins/ui_actions/public' {
export interface TriggerContextMapping {
[HELLO_WORLD_TRIGGER_ID]: undefined;
}

export interface ActionContextMapping {
[HELLO_WORLD_ACTION_TYPE]: undefined;
}
}

export class UiActionExamplesPlugin
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
17 changes: 7 additions & 10 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 @@ -34,32 +34,32 @@ 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 showcasePluggability = createAction({
export const showcasePluggability = createAction<typeof SHOWCASE_PLUGGABILITY_ACTION>({
type: SHOWCASE_PLUGGABILITY_ACTION,
getDisplayName: () => 'This is pluggable! Any plugin can inject their actions here.',
execute: async () => alert("Isn't that cool?!"),
});

export type PhoneContext = string;

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

export const lookUpWeatherAction = createAction<{ country: string }>({
export const lookUpWeatherAction = createAction<typeof TRAVEL_GUIDE_ACTION>({
type: TRAVEL_GUIDE_ACTION,
getIconType: () => 'popout',
getDisplayName: () => 'View travel guide',
execute: async ({ country }) => {
execute: async country => {
window.open(`https://www.worldtravelguide.net/?s=${country},`, '_blank');
stacey-gammon marked this conversation as resolved.
Show resolved Hide resolved
},
});

export type CountryContext = string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think will be better in this example to make all contexts into objects. And in general suggest it as best practice.

Right now from typescript perspective CountryContext and PhoneContext are the same: string. So we technically allowed to use country context where phone context is expected and other way around.

If we make it into {country: string} and {phone: string} it would be better and typescript won't allow to use one where other 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.

Yea, that's a good point. Plus makes it much easier to extend the interface without making breaking changes.

I can require it. It was originally required to be an object. What do you think? Require via typescript, or just encourage?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is not a big trouble to require it via typescript, I'd go for it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turned out to help anyway with the ts issue you spotted, so, done!


export const viewInMapsAction = createAction<CountryContext>({
export const viewInMapsAction = createAction<typeof VIEW_IN_MAPS_ACTION>({
type: VIEW_IN_MAPS_ACTION,
getIconType: () => 'popout',
getDisplayName: () => 'View in maps',
Expand Down Expand Up @@ -100,10 +100,7 @@ function EditUserModal({
}

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

export const createPhoneUserAction = (getUiActionsApi: () => Promise<UiActionsStart>) =>
createAction<UserContext>({
createAction<typeof PHONE_USER_ACTION>({
type: PHONE_USER_ACTION,
getDisplayName: () => 'Call phone number',
isCompatible: async ({ user }) => user.phone !== undefined,
Expand Down
9 changes: 5 additions & 4 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 @@ -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 HELLO_WORLD_ACTION_TYPE>({
id: `${HELLO_WORLD_ACTION_TYPE}-${name}`,
type: HELLO_WORLD_ACTION_TYPE,
getDisplayName: () => `Say hello to ${name}`,
execute: async () => {
const overlay = openModal(
Expand All @@ -95,10 +96,10 @@ 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(
{}
undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunate but typescript is requiring me to pass undefined explicitly

)}. Refresh the page to reset state. It's up to the user of the system to persist state like this.`
);
}}
Expand Down
46 changes: 26 additions & 20 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,
EDIT_USER_ACTION,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider turning ACTION_ into a prefix?

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 creates a pretty big commit to add to this PR, but sure.

SHOWCASE_PLUGGABILITY_ACTION,
CALL_PHONE_NUMBER_ACTION,
TRAVEL_GUIDE_ACTION,
VIEW_IN_MAPS_ACTION,
PHONE_USER_ACTION,
} 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 {
[EDIT_USER_ACTION]: UserContext;
[SHOWCASE_PLUGGABILITY_ACTION]: undefined;
[CALL_PHONE_NUMBER_ACTION]: PhoneContext;
[TRAVEL_GUIDE_ACTION]: CountryContext;
[VIEW_IN_MAPS_ACTION]: CountryContext;
[PHONE_USER_ACTION]: UserContext;
}
}

export class UiActionsExplorerPlugin implements Plugin<void, void, {}, StartDeps> {
Expand All @@ -67,29 +76,26 @@ 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
stacey-gammon marked this conversation as resolved.
Show resolved Hide resolved
// 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
12 changes: 6 additions & 6 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';

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,8 +44,8 @@ async function isCompatible(context: ActionContext) {
export function selectRangeAction(
filterManager: FilterManager,
timeFilter: TimefilterContract
): Action<ActionContext> {
return createAction<ActionContext>({
): ActionByType<typeof SELECT_RANGE_ACTION> {
return createAction<typeof SELECT_RANGE_ACTION>({
type: SELECT_RANGE_ACTION,
id: SELECT_RANGE_ACTION,
getDisplayName: () => {
Expand All @@ -54,7 +54,7 @@ export function selectRangeAction(
});
},
isCompatible,
execute: async ({ timeFieldName, data }: ActionContext) => {
execute: async ({ timeFieldName, data }: SelectRangeActionContext) => {
if (!(await isCompatible({ timeFieldName, data }))) {
throw new IncompatibleActionError();
}
Expand Down
12 changes: 6 additions & 6 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 @@ -39,12 +39,12 @@ import {

export const VALUE_CLICK_ACTION = 'VALUE_CLICK_ACTION';

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)) || [];
return filters.length > 0;
Expand All @@ -56,8 +56,8 @@ async function isCompatible(context: ActionContext) {
export function valueClickAction(
filterManager: FilterManager,
timeFilter: TimefilterContract
): Action<ActionContext> {
return createAction<ActionContext>({
): ActionByType<typeof VALUE_CLICK_ACTION> {
return createAction<typeof VALUE_CLICK_ACTION>({
type: VALUE_CLICK_ACTION,
id: VALUE_CLICK_ACTION,
getDisplayName: () => {
Expand All @@ -66,7 +66,7 @@ export function valueClickAction(
});
},
isCompatible,
execute: async ({ timeFieldName, data }: ActionContext) => {
execute: async ({ timeFieldName, data }: ValueClickActionContext) => {
if (!(await isCompatible({ timeFieldName, data }))) {
throw new IncompatibleActionError();
}
Expand Down
28 changes: 21 additions & 7 deletions src/legacy/core_plugins/data/public/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,16 @@ import {
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
} from '../../../../plugins/data/public/services';
import { setSearchServiceShim } from './services';
import { SELECT_RANGE_ACTION, selectRangeAction } from './actions/select_range_action';
import { VALUE_CLICK_ACTION, valueClickAction } from './actions/value_click_action';
import {
selectRangeAction,
SelectRangeActionContext,
SELECT_RANGE_ACTION,
} from './actions/select_range_action';
import {
valueClickAction,
VALUE_CLICK_ACTION,
ValueClickActionContext,
} from './actions/value_click_action';
import {
SELECT_RANGE_TRIGGER,
VALUE_CLICK_TRIGGER,
Expand Down Expand Up @@ -76,6 +84,12 @@ export interface DataSetup {
export interface DataStart {
search: SearchStart;
}
declare module '../../../../plugins/ui_actions/public' {
export interface ActionContextMapping {
[SELECT_RANGE_ACTION]: SelectRangeActionContext;
[VALUE_CLICK_ACTION]: ValueClickActionContext;
}
}

/**
* Data Plugin - public
Expand All @@ -100,10 +114,13 @@ export class DataPlugin
// This is to be deprecated once we switch to the new search service fully
addSearchStrategy(defaultSearchStrategy);

uiActions.registerAction(
uiActions.attachAction(
SELECT_RANGE_TRIGGER,
selectRangeAction(data.query.filterManager, data.query.timefilter.timefilter)
);
uiActions.registerAction(

uiActions.attachAction(
VALUE_CLICK_TRIGGER,
valueClickAction(data.query.filterManager, data.query.timefilter.timefilter)
);

Expand All @@ -123,9 +140,6 @@ export class DataPlugin
setSearchService(data.search);
setOverlays(core.overlays);

uiActions.attachAction(SELECT_RANGE_TRIGGER, SELECT_RANGE_ACTION);
uiActions.attachAction(VALUE_CLICK_TRIGGER, VALUE_CLICK_ACTION);

return {
search,
};
Expand Down
14 changes: 7 additions & 7 deletions src/plugins/dashboard_embeddable_container/public/actions/expand_panel_action.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import { i18n } from '@kbn/i18n';
import { IEmbeddable } from '../embeddable_plugin';
import { Action, IncompatibleActionError } from '../ui_actions_plugin';
import { ActionByType, IncompatibleActionError } from '../ui_actions_plugin';
import { DASHBOARD_CONTAINER_TYPE, DashboardContainer } from '../embeddable';

export const EXPAND_PANEL_ACTION = 'togglePanel';
Expand All @@ -36,18 +36,18 @@ function isExpanded(embeddable: IEmbeddable) {
return embeddable.id === embeddable.parent.getInput().expandedPanelId;
}

interface ActionContext {
export interface ExpandPanelActionContext {
embeddable: IEmbeddable;
}

export class ExpandPanelAction implements Action<ActionContext> {
export class ExpandPanelAction implements ActionByType<typeof EXPAND_PANEL_ACTION> {
public readonly type = EXPAND_PANEL_ACTION;
public readonly id = EXPAND_PANEL_ACTION;
public order = 7;

constructor() {}

public getDisplayName({ embeddable }: ActionContext) {
public getDisplayName({ embeddable }: ExpandPanelActionContext) {
if (!embeddable.parent || !isDashboard(embeddable.parent)) {
throw new IncompatibleActionError();
}
Expand All @@ -67,19 +67,19 @@ export class ExpandPanelAction implements Action<ActionContext> {
);
}

public getIconType({ embeddable }: ActionContext) {
public getIconType({ embeddable }: ExpandPanelActionContext) {
if (!embeddable.parent || !isDashboard(embeddable.parent)) {
throw new IncompatibleActionError();
}
// TODO: use 'minimize' when an eui-icon of such is available.
return isExpanded(embeddable) ? 'expand' : 'expand';
}

public async isCompatible({ embeddable }: ActionContext) {
public async isCompatible({ embeddable }: ExpandPanelActionContext) {
return Boolean(embeddable.parent && isDashboard(embeddable.parent));
}

public async execute({ embeddable }: ActionContext) {
public async execute({ embeddable }: ExpandPanelActionContext) {
if (!embeddable.parent || !isDashboard(embeddable.parent)) {
throw new IncompatibleActionError();
}
Expand Down
Loading