-
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
[Embeddable Rebuild] [Discover] Fix bundle size #189206
[Embeddable Rebuild] [Discover] Fix bundle size #189206
Conversation
a1c0bac
to
d107fac
Compare
/ci |
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
Thanks for looking into this! Just left one question/suggestion I think could be worth trying before approving.
const { apiCanAccessViewMode, apiHasType, apiIsOfType, getInheritedViewMode } = await import( | ||
'@kbn/presentation-publishing' | ||
); |
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.
One downside to this approach is that I think our async bundle will now grow with @kbn/presentation-publishing
since we're importing the entire package here. I wonder if it would work to keep the imports how they were for tree shaking, but break getCompatibilityCheck
out into a separate file and async import that here instead?
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.
Oh!! Genius 🤯 Good call on that one - did this in 5080816 and it reduced the async bundle size increase of this PR by about 50% (I think it was around +12KB
before, and it's +6KB
now).
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.
Awesome 🤘 thank you!
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.
LGTM, thanks for addressing this!
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @Heenawter |
…189533) #189128 increases @kbn/presentation-containers size. This causes synthetics page load bundle to exceed the bundle size limit. Synthetics page load should not include @kbn/presentation-containers. This PR resolves the problem by putting @kbn/presentation-containers load behind an async import Note for reviewers, put compatibility check into its own file to avoid importing entire @kbn/presentation-containers into async bundle. Tree shaking is enable by putting the import in a seperate file - see #189206 (comment) for more details
Summary
This PR is a follow up to #180536, where
8.8KB
was added to the Discover bundle size - now, after async importing from thepresentation-publishing
package where necessary, I've reduced that increase to3.4KB
(a decrease of5.4KB
from the previous PR).For maintainers