-
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
[Cases] Reducing the page load bundle size #157492
[Cases] Reducing the page load bundle size #157492
Conversation
Remove unnecessary changes.
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
Reverted async api imports.
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.
Great work!! 🎉
|
||
const FileAttachmentEvent = ({ file }: FileAttachmentEventProps) => { | ||
const { isPreviewVisible, showPreview, closePreview } = useFilePreview(); | ||
const FileAttachmentEvent = lazy(() => |
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.
Nice approach 👍
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 was effective because this attachment type is registered in x-pack/plugins/cases/public/plugin.ts
.
Before realizing I was trying to lazy load everything everywhere but it didn't work :D until I realized that the cases app is already mostly lazy loaded, these were the exceptions.
@@ -6,7 +6,7 @@ | |||
*/ | |||
|
|||
import * as rt from 'io-ts'; | |||
import { CaseStatuses } from '@kbn/cases-components'; | |||
import { CaseStatuses } from '@kbn/cases-components/src/status/types'; |
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.
Wasn't aware that not importing from exact folder increases bundle size 😄
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.
Ideally, it wouldn't because web pack should be able to recognize what is and is not necessary. But it seems we can't fully trust it 😬
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.
Haha.. better to be safe!! 👍
Added tests for file attachment event.
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @adcoelho |
Summary
Things got a bit out of control ever since we merged #154436.
In this PR I managed to reduce the cases bundle size from 166.0k to 140.0k.
This was done in several different steps:
Removing the usage of
query-string
This package took over 0.6% of the size of all chunks.
All functionality it provides could be replaced by the native URLSearchParans.
I removed the package in 1ef0211.
@kbn/cases-components
The cases plugin used imports from
@kbn-cases-components
in several places, these took up 4.4% of the bundle size.This would normally be fine but it seemed like
webpack
's tree shaking wasn't working correctly when doing imports from the top likeimport {...} from '@kbn/cases-components'
.The plugin bundle seemed to be getting too much unnecessary code so I changed the imports in 547755c to be more specific.
Now
@kbn/cases-components
only takes up 0.1% of the bundle size.Removing unnecessary public exports
I read the above in the documentation and checked what we exported. I found a bunch of things that we were exporting but that actually weren't being used by other plugins.
I removed those unnecessary exports in 6a0ff8c.
Unnecessary exports part 2
After merging the multiple file-related branches we were exporting the
imageMimeTypes
variable(a quite large array) twice. 53135ecIt belonged to
common/
that is referenced inextraPublicDirs
so that saved us a few more Ks.Cases plugin
In the cases plugin
start
we were returning a deprecated function that was not being used anywhere any more.I removed it -0.9K 🤷
42b30b3
Making the
clientAPI
lazyOnce again,
I was going over everything that our plugin exports to see if something was not used and noticed that the functions we return in
cases.api
are not used by everyone.They were already promises so I changed the implementation to lazy load the code.
A good rule of thumb here is that if the chunk is not always needed at setup/start time it might benefit from being lazy loaded. Some plugins need some of this code so we can
async import()
and reduce thepage load bundle size
significantly. See e201ab1.Lazy loading components in the file attachment
During the cases plugin setup step we register the file's internal attachment.
This defines how the file user action will look on the Case Detail page so naturally it requires a bunch of components to be loaded. The mistake here was that we don't need those components to be loaded right away.
I followed React's guidelines for lazy loading all components in the registered object greatly decreasing the bundle size in 1c9ede7.