Further improve type checking for actions and triggers #58765
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stacey-gammon
added
chore
release_note:skip
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
stacey-gammon
force-pushed
the
2020-02-26-action-types-2
branch
5 times, most recently
from
056d18b
to
fad8f45
Compare
stacey-gammon
commented
Feb 28, 2020
| setConfirmationText( | ||
| `You've successfully added a new action: ${dynamicAction.getDisplayName( | ||
| {} | ||
| undefined |
There was a problem hiding this comment.
unfortunate but typescript is requiring me to pass undefined explicitly
stacey-gammon
commented
Feb 28, 2020
| id: 'FOO', | ||
| type: 'FOO', | ||
| type: 'FOO' as ActionType, |
There was a problem hiding this comment.
If you don't want to do the declare module thing, like in tests, this casting as ActionType works.
stacey-gammon
commented
Feb 28, 2020
| import { UiComponent } from 'src/plugins/kibana_utils/common'; | ||
| import { ActionType, ActionContextMapping } from '../types'; | ||
|
|
||
| export interface ActionDefinition<T extends ActionType> { |
There was a problem hiding this comment.
Very duplicative of Action right now, but I'm using it in createAction function, and I know that the next PR will use ActionDefinition instead.
stacey-gammon
force-pushed
the
2020-02-26-action-types-2
branch
from
fad8f45
to
2ccd09f
Compare
stacey-gammon
force-pushed
the
2020-02-26-action-types-2
branch
from
2ccd09f
to
737f420
Compare
|
hit #58941 looks like. Maybe red on master? |
stacey-gammon
force-pushed
the
2020-02-26-action-types-2
branch
from
737f420
to
c25a97b
Compare
mattkime
approved these changes
Feb 28, 2020
stacey-gammon
force-pushed
the
2020-02-26-action-types-2
branch
from
c25a97b
to
223a9d3
Compare
lizozom
approved these changes
Mar 1, 2020
There was a problem hiding this comment.
Noticed something weird: Clicking on the "Make phone call action" from the "Row Actions" button, re-opens the menu at a new location, instead of showing the alert.
Also, if you click on the "Full screen" option in the table, the UI gets messed up and the only way to restore it, is by refreshing.
Other than, tested creating an addition action, which was very easy.
The PR itself is a text book example of Declaration Merging and I'm glad I got to review it!
Assuming those two issues are addressed \ handled in a follow PR, LGTM
| UserContext, | ||
| CountryContext, | ||
| PhoneContext, | ||
| EDIT_USER_ACTION, |
There was a problem hiding this comment.
Would you consider turning ACTION_ into a prefix?
There was a problem hiding this comment.
It creates a pretty big commit to add to this PR, but sure.
| @@ -36,10 +37,12 @@ const ReactMenuItem: React.FC = () => { | |||
|
|
|||
| const UiMenuItem = reactToUiComponent(ReactMenuItem); | |||
|
|
|||
| export const HELLO_WORLD_ACTION_ID = 'HELLO_WORLD_ACTION_ID'; | |||
| export const HELLO_WORLD_ACTION_ID = 'HELLO_WORLD_ACTION_ID' as ActionType; | |||
There was a problem hiding this comment.
Is this fully equivalent to the module declaration?
There was a problem hiding this comment.
No, it should be discouraged in a real situation, it's just a hack to get typescript to work. The context shape is just then the base type.
The problem with the jest tests like this is that I'm pretty sure I can't have two declare modules in the same plugin, one will override the other, and I was getting errors (and I def don't want to add this type mapping at the top level in prod code). So, real code should do the right thing, this is just for mocks/jest tests. I can add a comment on each of the places I am casting:
// Casting to ActionType is a hack - in a real situation use
// declare module and add this id to ActionContextMapping.
thoughts?
Dosant
reviewed
Mar 2, 2020
| 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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
Awesome! seems like working as expected 👍
| 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'); | ||
| }, | ||
| }); | ||
|
|
||
| export type CountryContext = string; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
If it is not a big trouble to require it via typescript, I'd go for it!
There was a problem hiding this comment.
Turned out to help anyway with the ts issue you spotted, so, done!
yea, this is expected, though unfortunate. I have a comment where I created the action. We want to do something similar with having CLICK_DATA_ACTION emit APPLY_FILTER_ACTION in certain cases so we need to resolve this. I left it in like this so we can use it as a test for how to solve the problem. I'm not yet sure how to solve. Maybe by extending the API so that instead of execute fn, an action can implement a different fn that returns an array of new context/trigger ids? So it's more of an "adapter" than an action. Haven't thought through very much...
This doesn't really have anything to do with ui actions, must have something to do with the new EUI table component. @chandlerprall - any ideas? Before:
After:
|
… type and thus the type issue
Dosant
approved these changes
Mar 3, 2020
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / X-Pack Spaces API Integration Tests -- security_and_spaces.x-pack/test/spaces_api_integration/security_and_spaces/apis/copy_to_space·ts.spaces api with security copy to spaces rbac user with all at space from the space_1 space "before each" hook for "should return 200 when copying to multiple spaces"Standard OutStack TraceKibana Pipeline / kibana-xpack-agent / X-Pack Spaces API Integration Tests -- security_and_spaces.x-pack/test/spaces_api_integration/security_and_spaces/apis/copy_to_space·ts.spaces api with security copy to spaces rbac user with all at space from the space_1 space "before each" hook for "should return 200 when copying to multiple spaces"Standard OutStack TraceKibana Pipeline / kibana-xpack-agent / X-Pack Spaces API Integration Tests -- security_and_spaces.x-pack/test/spaces_api_integration/security_and_spaces/apis/copy_to_space·ts.spaces api with security copy to spaces rbac user with all at space from the space_1 space "after each" hook for "should return 200 when copying to multiple spaces"Standard OutStack TraceHistory
To update your PR or re-run it, just comment with: |
stacey-gammon
added a commit
to stacey-gammon/kibana
that referenced
this pull request
Mar 4, 2020
* wip * review follow up * make ACTION a prefix, not SUFFIX * fix path * add warnings about casting to ActionType * Make context an object in examples, not a string * require object context, which seems to fix the partial requirement in type and thus the type issue * mistake Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
stacey-gammon
added a commit
that referenced
this pull request
Mar 5, 2020
* wip * review follow up * make ACTION a prefix, not SUFFIX * fix path * add warnings about casting to ActionType * Make context an object in examples, not a string * require object context, which seems to fix the partial requirement in type and thus the type issue * mistake Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will now provide warnings for attaching actions to triggers when required context is missing.
Builds on top of #58657