-
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
Lazy load Security plugin management apps. #64511
Conversation
Pinging @elastic/kibana-security (Team:Security) |
@@ -7,7 +7,6 @@ | |||
import { i18n } from '@kbn/i18n'; | |||
import { StartServicesAccessor, ApplicationSetup, AppMountParameters } from 'src/core/public'; | |||
import { AuthenticationServiceSetup } from '../authentication'; |
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.
note: decided to lazy load these API clients as well - not a big win, but doesn't cost too much either.
...ement/roles/edit_role/privileges/kibana/space_aware_privilege_section/_privilege_matrix.scss
Show resolved
Hide resolved
Thanks for taking this on @azasypkin! For reference, chunk
Based on a very quick analysis, my theory is that the IIRC, we only use the data plugin in order to retrieve a list of available index patterns, which drives the index privileges dropdown. |
Good find! Luckily it's not included into our main bundle that is loaded every time. |
I'll finish reviewing this once #63563 is finalized and incorporated into this PR, but so far this is looking good |
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.
Small comment on how the naming of the sass files will need to change. Core idea is great though. Thanks for making this change.
...k/plugins/security/public/management/roles/edit_role/collapsible_panel/collapsible_panel.tsx
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Done! The number of chunks tripled, but I'm not worried at this point as the main goal is to decrease size of the main bundle that is loaded every time for every app (decreased to one-tenth of its original size). If it becomes a problem in the future we may manually group chunks together (e.g. load all management apps in one big chunk). I'd not be happy about that level of fine tuning though. |
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.
LGreatTM, thanks for taking this on!
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.
Did a code review. LGTM.
7.x/7.8.0: 785c113 |
Summary
Measurements are based on this guide.
Before
- 2.7M - security.plugin.js - 149K - 1.plugin.js # I don't know where the chunk `0` is ..... - 175K - 2.plugin.js - 135K - 3.plugin.js - 32K - 4.plugin.js - 13K - 5.plugin.js - 3.2K - 6.plugin.js - 6.1K - 7.plugin.js - 15K - 8.plugin.js
After
Related to: #64192
cc @restrry