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

[Response Ops][Rule Form V2] Move dependencies from triggers actions UI to shared package #184977

Conversation

JiaweiWu
Copy link
Contributor

@JiaweiWu JiaweiWu commented Jun 7, 2024

Summary

Issue: #179105
Related PR: #180539

Part 2.5/3 PRs of the new rule form. This PR acts as the foundation PR for the main rule form PR by moving a lot of the dependencies needed by the rules form to a shared package. So no new features added, just moving stuff around.

Checklist

@JiaweiWu JiaweiWu added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.15.0 labels Jun 7, 2024
@JiaweiWu JiaweiWu requested a review from a team as a code owner June 7, 2024 03:07
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Moving all this code to the alerts-ui-shared package will grow its size by a lot and also the bundle size of plugins that consume it. I suggest splitting up the code into multiple packages by following the new convention outlined in the dev docs.

- packages/response-ops
    - hooks-browser (@kbn/response-ops-hooks-browser)
    - api-browser (@kbn/response-ops-api-browser)

@JiaweiWu
Copy link
Contributor Author

JiaweiWu commented Jun 7, 2024

@elasticmachine merge upstream

@JiaweiWu
Copy link
Contributor Author

JiaweiWu commented Jun 7, 2024

@elasticmachine merge upstream

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

LGTM! We discussed offline that we will come up with a naming convention and strategy for our packages before splitting the alerts-ui-shared package into multiple ones.

@JiaweiWu
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
alerting 179 182 +3
observability 491 522 +31
securitySolution 5507 5509 +2
triggersActionsUi 736 761 +25
total +61

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
@kbn/alerting-types 176 187 +11
@kbn/alerts-ui-shared 104 223 +119
alerting 837 836 -1
total +129

Async chunks

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

id before after diff
observability 308.6KB 308.6KB +46.0B
triggersActionsUi 1.6MB 1.7MB +14.5KB
total +14.6KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/alerts-ui-shared 3 2 -1
triggersActionsUi 58 52 -6
total -7

Page load bundle

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

id before after diff
alerting 25.1KB 24.9KB -129.0B
triggersActionsUi 114.5KB 114.4KB -107.0B
total -236.0B
Unknown metric groups

API count

id before after diff
@kbn/alerting-types 179 190 +11
@kbn/alerts-ui-shared 116 237 +121
alerting 869 868 -1
total +131

ESLint disabled line counts

id before after diff
@kbn/alerts-ui-shared 6 8 +2
triggersActionsUi 127 126 -1
total +1

Total ESLint disabled count

id before after diff
@kbn/alerts-ui-shared 6 8 +2
triggersActionsUi 133 132 -1
total +1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@JiaweiWu JiaweiWu merged commit a25ccec into elastic:main Jun 12, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants