-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Conversation
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
Pinging @elastic/kibana-presentation (Team:Presentation) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this 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 👍
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); | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved with 7ac06f4
@elasticmachine merge upstream |
There was a problem hiding this 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
There was a problem hiding this 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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
src/platform/plugins/private/presentation_panel/public/panel_component/panel_module.ts
Show resolved
Hide resolved
src/platform/plugins/shared/ui_actions/public/service/ui_actions_service.ts
Show resolved
Hide resolved
There was a problem hiding this 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.
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
|
Starting backport for target branches: 8.x |
💔 All backports failed
Manual backportTo create the backport manually run:
Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
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
# 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-->
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]>
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]>
Closes #191642, part of #205512
PR makes the following changes to uiActions API.
registerAction
andaddTriggerAction
and replaces them withregisterActionAsync
andaddTriggerActionAsync
getAction
,getTriggerActions
andgetFrequentlyChangingActionsForTrigger
PR updates presentation_panel plugin to use
registerActionAsync
. With actions behind async import, page load bundle size has been reduced by 21.1KB.Also, async exports are contained in a single file
panel_module
. This results in dashboard only loading onepresentationPanel.chunk
.