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.

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

[uiActions] make trigger action registry async #205512

Merged
merged 36 commits into from
Jan 13, 2025
Merged

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jan 3, 2025

Closes #191642, part of #205512

PR makes the following changes to uiActions API.

  • Deprecates registerAction and addTriggerAction and replaces them with registerActionAsync and addTriggerActionAsync
  • Makes all registry consumption methods async, such as getAction, getTriggerActions and getFrequentlyChangingActionsForTrigger

PR updates presentation_panel plugin to use registerActionAsync. With actions behind async import, page load bundle size has been reduced by 21.1KB.

Screenshot 2025-01-08 at 2 14 23 PM

Also, async exports are contained in a single file panel_module. This results in dashboard only loading one presentationPanel.chunk.

Screenshot 2025-01-08 at 2 15 02 PM

@nreese
Copy link
Contributor Author

nreese commented Jan 3, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Jan 3, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Jan 3, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Jan 3, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Jan 3, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Jan 3, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Jan 4, 2025

/ci

@nreese
Copy link
Contributor Author

nreese commented Jan 4, 2025

/ci

@nreese nreese added backport:version Backport to applied version labels v8.18.0 labels Jan 8, 2025
@elasticmachine
Copy link
Contributor

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

@ThomThomson ThomThomson self-requested a review January 8, 2025 21:18
@nreese
Copy link
Contributor Author

nreese commented Jan 8, 2025

@elasticmachine merge upstream

@nreese
Copy link
Contributor Author

nreese commented Jan 8, 2025

@elasticmachine merge upstream

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Checked quickly in Discover and no issues, Data Discovery changes LGTM 👍

Comment on lines 41 to 51
const results = await Promise.allSettled([
startServicesPromise,
componentPromise,
modulePromise,
import('./panel_module'),
]);
const Panel = panelModule.PresentationPanelInternal;
return { Panel, unwrappedComponent };

for (const result of results) {
if (result.status === 'rejected') {
throw new Error(result.reason);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: all this code looks very similar to the default behaviour of Promise.all? Why checking it manually?

Copy link
Contributor Author

@nreese nreese Jan 10, 2025

Choose a reason for hiding this comment

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

Promise.all does not return values when any promise rejects. Since PresentationPanelErrorInternal is returned from one of the promises (and PresentationPanelErrorInternal is used to diplay errors), I am using Promise. allSettled so PresentationPanelErrorInternal is returned if componentPromise rejects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do see a logic error now where the method throws when a promise rejects so no values are returned. I will rework this so that it does not throw on rejected, and instead returns an error status along with any available results

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved with 7ac06f4

@nreese
Copy link
Contributor Author

nreese commented Jan 10, 2025

@elasticmachine merge upstream

Copy link
Contributor

@eokoneyo eokoneyo left a comment

Choose a reason for hiding this comment

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

Shared UX changes LGTM, thanks for the contribution

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

This is such a straightforward set of changes for such an impactful reduction in bundle size! Excellent idea and execution here.

Looked through the code and everything LGTM!

Also ran this locally and verified that the presentationPanel plugin had only one async chunk, and that it was loaded when expected (i.e. when a Dashboard with at least one panel is on screen).

@@ -16,49 +16,37 @@ export function AddButton({ pageApi, uiActions }: { pageApi: unknown; uiActions:
const [items, setItems] = useState<ReactElement[]>([]);

useEffect(() => {
let cancelled = false;
let canceled = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Canadian vs American spelling here 😈 CC @Heenawter for backup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that, reverted here acba4f1

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

ML related changes LGTM. This is a nice enhancement, we're looking forward to make use of it.

@nreese
Copy link
Contributor Author

nreese commented Jan 13, 2025

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
presentationPanel 104 117 +13

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
presentationPanel 11 9 -2
uiActions 110 117 +7
total +5

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dataVisualizer 598.5KB 598.5KB +13.0B
discover 843.2KB 843.3KB +16.0B
lens 1.6MB 1.6MB +30.0B
maps 3.1MB 3.1MB +40.0B
presentationPanel 15.6KB 35.9KB +20.3KB
total +20.4KB

Page load bundle

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

id before after diff
dashboard 49.0KB 49.2KB +163.0B
presentationPanel 44.9KB 23.9KB -21.0KB
presentationUtil 32.4KB 32.4KB +11.0B
uiActions 21.5KB 22.0KB +564.0B
total -20.3KB
Unknown metric groups

API count

id before after diff
presentationPanel 11 9 -2
uiActions 156 163 +7
total +5

ESLint disabled line counts

id before after diff
dashboard 10 11 +1
presentationPanel 6 7 +1
total +2

References to deprecated APIs

id before after diff
aiops 4 9 +5
cases 31 32 +1
controls 13 16 +3
dashboard 21 30 +9
dashboardEnhanced 9 11 +2
data 29 32 +3
discover 8 10 +2
discoverEnhanced 0 2 +2
esql 0 1 +1
imageEmbeddable 1 2 +1
inputControlVis 10 11 +1
lens 85 92 +7
links 3 4 +1
maps 30 33 +3
ml 35 53 +18
reporting 4 5 +1
securitySolution 355 362 +7
slo 3 7 +4
synthetics 20 22 +2
uiActionsEnhanced 11 12 +1
unifiedSearch 13 15 +2
visualizations 55 57 +2
total +78

Total ESLint disabled count

id before after diff
dashboard 10 11 +1
presentationPanel 6 7 +1
total +2

History

@nreese nreese merged commit 1bd8c97 into elastic:main Jan 13, 2025
8 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12752165266

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 205512

Questions ?

Please refer to the Backport tool documentation

@nreese
Copy link
Contributor Author

nreese commented Jan 13, 2025

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

nreese added a commit to nreese/kibana that referenced this pull request Jan 13, 2025
Closes elastic#191642, part of
elastic#205512

PR makes the following changes to uiActions API.
* Deprecates `registerAction` and `addTriggerAction` and replaces them
with `registerActionAsync` and `addTriggerActionAsync`
* Makes all registry consumption methods async, such as `getAction`,
`getTriggerActions` and `getFrequentlyChangingActionsForTrigger`

PR updates presentation_panel plugin to use `registerActionAsync`. With
actions behind async import, page load bundle size has been reduced by
21.1KB.

<img width="500" alt="Screenshot 2025-01-08 at 2 14 23 PM"
src="https://github.com/user-attachments/assets/34a2cae9-dc5e-429b-bbdb-ffd9dfe1cce3"
/>

Also, async exports are [contained in a single file
`panel_module`](elastic#206117). This
results in dashboard only loading one `presentationPanel.chunk`.

<img width="500" alt="Screenshot 2025-01-08 at 2 15 02 PM"
src="https://github.com/user-attachments/assets/e083b852-b50d-4fa7-8ebd-e2f56f85e998"
/>

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 1bd8c97)

# Conflicts:
#	src/platform/plugins/private/presentation_panel/public/panel_component/presentation_panel.tsx
nreese added a commit that referenced this pull request Jan 13, 2025
# Backport

This will backport the following commits from `main` to `8.x`:
- [[uiActions] make trigger action registry async
(#205512)](#205512)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Nathan
Reese","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-01-13T16:57:34Z","message":"[uiActions]
make trigger action registry async (#205512)\n\nCloses
#191642, part
of\r\nhttps://github.com//pull/205512\r\n\r\nPR makes the
following changes to uiActions API.\r\n* Deprecates `registerAction` and
`addTriggerAction` and replaces them\r\nwith `registerActionAsync` and
`addTriggerActionAsync`\r\n* Makes all registry consumption methods
async, such as `getAction`,\r\n`getTriggerActions` and
`getFrequentlyChangingActionsForTrigger`\r\n\r\nPR updates
presentation_panel plugin to use `registerActionAsync`. With\r\nactions
behind async import, page load bundle size has been reduced
by\r\n21.1KB.\r\n\r\n<img width=\"500\" alt=\"Screenshot 2025-01-08 at 2
14
23 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/34a2cae9-dc5e-429b-bbdb-ffd9dfe1cce3\"\r\n/>\r\n\r\nAlso,
async exports are [contained in a single
file\r\n`panel_module`](#206117).
This\r\nresults in dashboard only loading one
`presentationPanel.chunk`.\r\n\r\n<img width=\"500\" alt=\"Screenshot
2025-01-08 at 2 15
02 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/e083b852-b50d-4fa7-8ebd-e2f56f85e998\"\r\n/>\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"1bd8c97982146a4643d6eca1e21b525a5c8d456f","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Presentation","release_note:skip","v9.0.0","backport:version","v8.18.0"],"number":205512,"url":"https://github.com/elastic/kibana/pull/205512","mergeCommit":{"message":"[uiActions]
make trigger action registry async (#205512)\n\nCloses
#191642, part
of\r\nhttps://github.com//pull/205512\r\n\r\nPR makes the
following changes to uiActions API.\r\n* Deprecates `registerAction` and
`addTriggerAction` and replaces them\r\nwith `registerActionAsync` and
`addTriggerActionAsync`\r\n* Makes all registry consumption methods
async, such as `getAction`,\r\n`getTriggerActions` and
`getFrequentlyChangingActionsForTrigger`\r\n\r\nPR updates
presentation_panel plugin to use `registerActionAsync`. With\r\nactions
behind async import, page load bundle size has been reduced
by\r\n21.1KB.\r\n\r\n<img width=\"500\" alt=\"Screenshot 2025-01-08 at 2
14
23 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/34a2cae9-dc5e-429b-bbdb-ffd9dfe1cce3\"\r\n/>\r\n\r\nAlso,
async exports are [contained in a single
file\r\n`panel_module`](#206117).
This\r\nresults in dashboard only loading one
`presentationPanel.chunk`.\r\n\r\n<img width=\"500\" alt=\"Screenshot
2025-01-08 at 2 15
02 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/e083b852-b50d-4fa7-8ebd-e2f56f85e998\"\r\n/>\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"1bd8c97982146a4643d6eca1e21b525a5c8d456f"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/205512","number":205512,"mergeCommit":{"message":"[uiActions]
make trigger action registry async (#205512)\n\nCloses
#191642, part
of\r\nhttps://github.com//pull/205512\r\n\r\nPR makes the
following changes to uiActions API.\r\n* Deprecates `registerAction` and
`addTriggerAction` and replaces them\r\nwith `registerActionAsync` and
`addTriggerActionAsync`\r\n* Makes all registry consumption methods
async, such as `getAction`,\r\n`getTriggerActions` and
`getFrequentlyChangingActionsForTrigger`\r\n\r\nPR updates
presentation_panel plugin to use `registerActionAsync`. With\r\nactions
behind async import, page load bundle size has been reduced
by\r\n21.1KB.\r\n\r\n<img width=\"500\" alt=\"Screenshot 2025-01-08 at 2
14
23 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/34a2cae9-dc5e-429b-bbdb-ffd9dfe1cce3\"\r\n/>\r\n\r\nAlso,
async exports are [contained in a single
file\r\n`panel_module`](#206117).
This\r\nresults in dashboard only loading one
`presentationPanel.chunk`.\r\n\r\n<img width=\"500\" alt=\"Screenshot
2025-01-08 at 2 15
02 PM\"\r\nsrc=\"https://github.com/user-attachments/assets/e083b852-b50d-4fa7-8ebd-e2f56f85e998\"\r\n/>\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>\r\nCo-authored-by:
Elastic Machine
<[email protected]>","sha":"1bd8c97982146a4643d6eca1e21b525a5c8d456f"}},{"branch":"8.x","label":"v8.18.0","labelRegex":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 13, 2025
Closes elastic#191642, part of
elastic#205512

PR makes the following changes to uiActions API.
* Deprecates `registerAction` and `addTriggerAction` and replaces them
with `registerActionAsync` and `addTriggerActionAsync`
* Makes all registry consumption methods async, such as `getAction`,
`getTriggerActions` and `getFrequentlyChangingActionsForTrigger`

PR updates presentation_panel plugin to use `registerActionAsync`. With
actions behind async import, page load bundle size has been reduced by
21.1KB.

<img width="500" alt="Screenshot 2025-01-08 at 2 14 23 PM"
src="https://github.com/user-attachments/assets/34a2cae9-dc5e-429b-bbdb-ffd9dfe1cce3"
/>

Also, async exports are [contained in a single file
`panel_module`](elastic#206117). This
results in dashboard only loading one `presentationPanel.chunk`.

<img width="500" alt="Screenshot 2025-01-08 at 2 15 02 PM"
src="https://github.com/user-attachments/assets/e083b852-b50d-4fa7-8ebd-e2f56f85e998"
/>

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
Closes elastic#191642, part of
elastic#205512

PR makes the following changes to uiActions API.
* Deprecates `registerAction` and `addTriggerAction` and replaces them
with `registerActionAsync` and `addTriggerActionAsync`
* Makes all registry consumption methods async, such as `getAction`,
`getTriggerActions` and `getFrequentlyChangingActionsForTrigger`

PR updates presentation_panel plugin to use `registerActionAsync`. With
actions behind async import, page load bundle size has been reduced by
21.1KB.

<img width="500" alt="Screenshot 2025-01-08 at 2 14 23 PM"
src="https://github.com/user-attachments/assets/34a2cae9-dc5e-429b-bbdb-ffd9dfe1cce3"
/>

Also, async exports are [contained in a single file
`panel_module`](elastic#206117). This
results in dashboard only loading one `presentationPanel.chunk`.

<img width="500" alt="Screenshot 2025-01-08 at 2 15 02 PM"
src="https://github.com/user-attachments/assets/e083b852-b50d-4fa7-8ebd-e2f56f85e998"
/>

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[uiActions] make trigger action registry async
8 participants