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

[uiActions] make trigger action registry async #191642

Open
nreese opened this issue Aug 28, 2024 · 4 comments · Fixed by #195616 · May be fixed by #205512
Open

[uiActions] make trigger action registry async #191642

nreese opened this issue Aug 28, 2024 · 4 comments · Fixed by #195616 · May be fixed by #205512
Assignees
@nreese
Labels
Feature:UIActions UI actions. These are client side only, not related to the server side actions.. Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) technical debt Improvement of the software architecture and operational architecture

Comments

@nreese
Copy link
Contributor

nreese commented Aug 28, 2024 •
edited
Loading

The current trigger action registry takes the parameters triggerId: string, action: ActionDefinition<any>. This requires that the action is included in the page load bundle.

Instead, the registry should take the parameters triggerId: string, actionId: string, getAction: () => Promise(ActionDefinition<any>). An example of a registry with an async getter is the react embeddable registry.

Current registry example

Below is an example of an existing action. The action is from maps plugin and is used to synchronize map movement in a dashboard.
Notice in the action how the author has to be careful to async import isCompatible and execute implementations to avoid bloating page load bundle. This very error prone and many times, actions are a source of page load bloat. Even with care, the action definition itself is still included in the page load bundle.

Example trigger action registration
plugins.uiActions.addTriggerAction(CONTEXT_MENU_TRIGGER, synchronizeMovementAction);
Example action
export const synchronizeMovementAction = createAction<EmbeddableApiContext>({
  id: SYNCHRONIZE_MOVEMENT_ACTION,
  type: SYNCHRONIZE_MOVEMENT_ACTION,
  order: 21,
  getDisplayName: () => {
    return i18n.translate('xpack.maps.synchronizeMovementAction.title', {
      defaultMessage: 'Synchronize map movement',
    });
  },
  getDisplayNameTooltip: () => {
    return i18n.translate('xpack.maps.synchronizeMovementAction.tooltipContent', {
      defaultMessage:
        'Synchronize maps, so that if you zoom and pan in one map, the movement is reflected in other maps',
    });
  },
  getIconType: () => {
    return 'crosshairs';
  },
  isCompatible: async ({ embeddable }: EmbeddableApiContext) => {
    if (!isApiCompatible(embeddable)) return false;
    const { isCompatible } = await import('./is_compatible');
    return isCompatible(embeddable);
  },
  execute: async () => {
    const { openModal } = await import('./modal');
    openModal();
  },
});

Proposed registry example

Instead, the registry should take an async getter so that the action is not included in the page load bundle and instead is only loaded when the trigger or action id is required.

Example trigger action registration
plugins.uiActions.addTriggerAction(CONTEXT_MENU_TRIGGER, SYNCHRONIZE_MOVEMENT_ACTION, async () => {
  const { synchronizeMovementAction } = await import(
     './actions/synchronize_movement_action'
  );
  return synchronizeMovementAction;
});
@nreese nreese added the technical debt Improvement of the software architecture and operational architecture label Aug 28, 2024
@botelastic botelastic bot added the needs-team Issues missing a team label label Aug 28, 2024
@nreese nreese added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label Aug 28, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@botelastic botelastic bot removed the needs-team Issues missing a team label label Aug 28, 2024
@nreese nreese added the Feature:UIActions UI actions. These are client side only, not related to the server side actions.. label Aug 28, 2024
@nreese nreese mentioned this issue Oct 9, 2024
Merged
nreese added a commit that referenced this issue Oct 9, 2024
@nreese
[dashboard] do not async import dashboard actions on plugin load (#19…
7448376 
…5616)

Part of #194171

Notice in the screen shot below, how opening Kibana home page loads
async dashboard chunks.
<img width="500" alt="Screenshot 2024-10-09 at 8 38 24 AM"
src="https://github.com/user-attachments/assets/2cfdb512-03e4-4634-bb0c-a8d163f98540">

This PR replaces async import with a sync import. The page load bundle
size increases but this is no different than the current behavior, just
the import is now properly accounted for in page load stats. The next
step is to resolve #191642 and
reduce the page load impact of registering uiActions (but this is out of
scope for this PR).
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Oct 9, 2024
@nreese
[dashboard] do not async import dashboard actions on plugin load (ela…
83d62c1 
…stic#195616)

Part of elastic#194171

Notice in the screen shot below, how opening Kibana home page loads
async dashboard chunks.
<img width="500" alt="Screenshot 2024-10-09 at 8 38 24 AM"
src="https://github.com/user-attachments/assets/2cfdb512-03e4-4634-bb0c-a8d163f98540">

This PR replaces async import with a sync import. The page load bundle
size increases but this is no different than the current behavior, just
the import is now properly accounted for in page load stats. The next
step is to resolve elastic#191642 and
reduce the page load impact of registering uiActions (but this is out of
scope for this PR).

(cherry picked from commit 7448376)
kibanamachine added a commit that referenced this issue Oct 9, 2024
@kibanamachine @nreese
[8.x] [dashboard] do not async import dashboard actions on plugin load (
884b861 
#195616) (#195655)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[dashboard] do not async import dashboard actions on plugin load
(#195616)](#195616)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-09T17:21:15Z","message":"[dashboard]
do not async import dashboard actions on plugin load (#195616)\n\nPart
of https://github.com/elastic/kibana/issues/194171\r\n\r\nNotice in the
screen shot below, how opening Kibana home page loads\r\nasync dashboard
chunks.\r\n<img width=\"500\" alt=\"Screenshot 2024-10-09 at 8 38
24 AM\"\r\nsrc=\"https://github.com/user-attachments/assets/2cfdb512-03e4-4634-bb0c-a8d163f98540\">\r\n\r\nThis
PR replaces async import with a sync import. The page load
bundle\r\nsize increases but this is no different than the current
behavior, just\r\nthe import is now properly accounted for in page load
stats. The next\r\nstep is to resolve
#191642 and\r\nreduce the page
load impact of registering uiActions (but this is out of\r\nscope for
this
PR).","sha":"7448376119aa1aad0888eb68449c1b15fd0852ac","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Presentation","release_note:skip","v9.0.0","v8.16.0","backport:version"],"title":"[dashboard]
do not async import dashboard actions on plugin
load","number":195616,"url":"https://github.com/elastic/kibana/pull/195616","mergeCommit":{"message":"[dashboard]
do not async import dashboard actions on plugin load (#195616)\n\nPart
of https://github.com/elastic/kibana/issues/194171\r\n\r\nNotice in the
screen shot below, how opening Kibana home page loads\r\nasync dashboard
chunks.\r\n<img width=\"500\" alt=\"Screenshot 2024-10-09 at 8 38
24 AM\"\r\nsrc=\"https://github.com/user-attachments/assets/2cfdb512-03e4-4634-bb0c-a8d163f98540\">\r\n\r\nThis
PR replaces async import with a sync import. The page load
bundle\r\nsize increases but this is no different than the current
behavior, just\r\nthe import is now properly accounted for in page load
stats. The next\r\nstep is to resolve
#191642 and\r\nreduce the page
load impact of registering uiActions (but this is out of\r\nscope for
this
PR).","sha":"7448376119aa1aad0888eb68449c1b15fd0852ac"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/195616","number":195616,"mergeCommit":{"message":"[dashboard]
do not async import dashboard actions on plugin load (#195616)\n\nPart
of https://github.com/elastic/kibana/issues/194171\r\n\r\nNotice in the
screen shot below, how opening Kibana home page loads\r\nasync dashboard
chunks.\r\n<img width=\"500\" alt=\"Screenshot 2024-10-09 at 8 38
24 AM\"\r\nsrc=\"https://github.com/user-attachments/assets/2cfdb512-03e4-4634-bb0c-a8d163f98540\">\r\n\r\nThis
PR replaces async import with a sync import. The page load
bundle\r\nsize increases but this is no different than the current
behavior, just\r\nthe import is now properly accounted for in page load
stats. The next\r\nstep is to resolve
#191642 and\r\nreduce the page
load impact of registering uiActions (but this is out of\r\nscope for
this
PR).","sha":"7448376119aa1aad0888eb68449c1b15fd0852ac"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Nathan Reese <[email protected]>
@nreese nreese reopened this Oct 24, 2024
@nreese
Copy link
Contributor Author

nreese commented Oct 24, 2024

#195616 did not close issue. It is just part of this effort

@Heenawter Heenawter added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Nov 14, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@Heenawter Heenawter removed the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Dec 4, 2024
@nreese
Copy link
Contributor Author

nreese commented Dec 4, 2024

Breaking @elastic/kibana-presentation effort into #202986 proposal

@nreese nreese mentioned this issue Dec 4, 2024
Closed
@nreese nreese self-assigned this Jan 3, 2025
@nreese nreese added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Jan 3, 2025
@nreese nreese linked a pull request Jan 3, 2025 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nreese nreese

Labels
Feature:UIActions UI actions. These are client side only, not related to the server side actions.. Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Milestone
No milestone
3 participants
@nreese @Heenawter @elasticmachine