-
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
[bundle optimization] Update to semver 7.x to get tree-shaking #83020
Conversation
@tylersmalley @spalger I added you as reviewers to get any thoughts before I marked this ready for review and pinged other teams. Let me know if I should keep going, take a different approach, hold off entirely, or anything else that comes to mind. Thanks. |
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.
No objection to the upgrade, but I don't love the changes to imports. Relying on users to know that they should import the inner modules feels like it's setup to fail. It would be ideal if we could get this enforced with the module-migration
custom eslint rule. If it's not worth enforcing then I wonder if it's worth making the change.
@spalger The import changes aren't strictly required to upgrade to 7.x. People can keep doing what they're doing now if they wish. They'll continue to pull in more than required but the library is a bit larger now so the amount of extra code is greater. The initial commit which only upgraded I updated the paths because I thought a PR with ~10K/20% savings was more attractive than the alternative 10K/20% increase but it's definitely a team decision. I just wanted to make it easy to get the savings without having to do the work of updating the paths themselves. |
Ah, okay, in that case I understand the justification for the extra changes regardless of whether we enforce them or not. |
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 to see the change in the bundle size 🚀
Pinging @elastic/ingest-management (Team:Ingest Management) |
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.
Looks like the version number logic in Upgrade Assistant is unaffected. Code LGTM.
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 from Enterprise Search 👍
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 changes LGTM
pinging @elastic/kibana-presentation @elastic/kibana-telemetry for 👀 when you have a moment |
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.
Kibana Presentation changes LGTM. Seems to be just imports / usage of semver. Code only review
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.
Telemetry-related changes LGTM. Nice work on the bundle sizes 😄
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
Page load bundle
History
To update your PR or re-run it, just comment with: |
* master: Migrate `/translations` route to core (elastic#83280) [APM] Ensure APM jest script can run (elastic#83398) [Uptime] Monitor status alert use url as instance (elastic#81736) [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259) TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964) [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139) Skips Vega test skip flaky suite (elastic#79967) [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020) Added ability to fire actions when an alert instance is resolved (elastic#82799) [ML] Adds functional tests for the index data visualizer card contents (elastic#83174)
… into add-logs-to-node-details * 'add-logs-to-node-details' of github.com:phillipb/kibana: fix tall vislib charts in visualize (elastic#83340) [Lens] Avoid unnecessary data fetching on dimension flyout open (elastic#82957) [Security Solution][Case] Change case connector minimum required license to basic (elastic#83401) fix logstash central pipeline management test (elastic#83281) [Search] Send to background UI (elastic#81793) Migrate `/translations` route to core (elastic#83280) [APM] Ensure APM jest script can run (elastic#83398) [Uptime] Monitor status alert use url as instance (elastic#81736) [ML] Add basic license test run details to ML+Transform READMEs (elastic#83259) TSVB doesn't communicate it's index-patterns to dashboard (elastic#82964) [Alerting UI] Added ability to assign alert actions to resolved action group in UI (elastic#83139) Skips Vega test skip flaky suite (elastic#79967) [bundle optimization] Update to semver 7.x to get tree-shaking (elastic#83020) Added ability to fire actions when an alert instance is resolved (elastic#82799)
Friendly reminder: Looks like this PR hasn’t been backported yet. |
…ic#83020) ## What's changed in this PR ### Update to latest available `semver`: `7.3.2` * `semver` 5.x pulls in the entire library in one large file (~38k uncompressed / ~9k gz), when we might only use 1-2K. * `semver` 7.0+ supports tree-shaking: https://github.com/npm/node-semver/blob/master/CHANGELOG.md#700 ### Update paths to only import individual function(s) used instead of the entire library * Getting the smaller bundle requires a different import style [as shown in the docs](https://github.com/npm/node-semver#usage) * Only changed code in `public` & `common` folders; not `server`. We could also update `server` as well for consistency, but I skipped because the new import style is more verbose and the filesize didn't seem as important on the server ### Results The build stats show a 10K+ improvement for initial page bundles elastic#83020 (comment) | id | [before](elastic@c6afc47) | [after](elastic@213bb52) | diff | | --- | --- | --- | --- | | `ingestManager` | 386.2KB | 373.9KB | -12.3KB | | `telemetry` | 63.5KB | 50.1KB | -13.5KB | | `upgradeAssistant` | 74.5KB | 60.5KB | -14.0KB | | total | | | -39.7KB | ### The import paths look odd. Are they required? I agree and, no, they're not strictly required. If you'd like me to revert to the prior style just drop a comment and I'll undo them. The caveat is that the current style (in `master` & this PR) pulls in the entire `semver` library. In 7.x that added ~15K to the initial size. Some more details in the comments: elastic#83020 (comment) ### Possible issues Moving 2 major versions. We're currently on 5.7 and the latest available is 7.3. * changelog says 5.x (our current) to 6.0 should be safe: https://github.com/npm/node-semver/blob/master/CHANGELOG.md#60 * There 6.x & 7.x changes all appear to be new features or bugfixes around the `includePrerelease` flag added in 5.6, but I'm not sure if those "fixes" will break existing code * https://github.com/npm/node-semver/blob/master/CHANGELOG.md#613 * https://github.com/npm/node-semver/blob/master/CHANGELOG.md#722 ### Stats / screenshots generated with `node scripts/build_kibana_platform_plugins.js --profile --focus=ingestManager` <details><summary><b>Ingest Manager in `master`</b>: imports entire `semver` lib, totals 40k+, only 1 large file (orange arc below)</summary> <img width="972" alt="Screen Shot 2020-11-09 at 6 50 23 PM" src="https://user-images.githubusercontent.com/57655/98666188-a50ac380-231a-11eb-9b8a-6ca784752714.png"> </details> <details><summary><b>Ingest Manager in PR after upgrade to 7</b>: still imports entire lib. file size *increased* to ~60k, but now individual files are imported (orange arcs below)</summary> <img width="825" alt="Screen Shot 2020-11-09 at 5 46 30 PM" src="https://user-images.githubusercontent.com/57655/98666355-e602d800-231a-11eb-803f-bc04beb4eaf1.png"> <img width="963" alt="Screen Shot 2020-11-09 at 5 47 06 PM" src="https://user-images.githubusercontent.com/57655/98666357-e69b6e80-231a-11eb-92d3-c66904f92c30.png"> </details> <details><summary><b>Ingest Manager in PR after changing `import`s:</b> total imported size down to ~20k. Can see individual imported files</summary> <img width="926" alt="Screen Shot 2020-11-10 at 6 10 23 AM" src="https://user-images.githubusercontent.com/57655/98667058-e64fa300-231b-11eb-9690-5e36ed6475e0.png"> <img width="895" alt="Screen Shot 2020-11-10 at 6 10 53 AM" src="https://user-images.githubusercontent.com/57655/98667059-e780d000-231b-11eb-8abf-98d8bdbcf061.png"> </details> ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
… (#83542) ## What's changed in this PR ### Update to latest available `semver`: `7.3.2` * `semver` 5.x pulls in the entire library in one large file (~38k uncompressed / ~9k gz), when we might only use 1-2K. * `semver` 7.0+ supports tree-shaking: https://github.com/npm/node-semver/blob/master/CHANGELOG.md#700 ### Update paths to only import individual function(s) used instead of the entire library * Getting the smaller bundle requires a different import style [as shown in the docs](https://github.com/npm/node-semver#usage) * Only changed code in `public` & `common` folders; not `server`. We could also update `server` as well for consistency, but I skipped because the new import style is more verbose and the filesize didn't seem as important on the server ### Results The build stats show a 10K+ improvement for initial page bundles #83020 (comment) | id | [before](c6afc47) | [after](213bb52) | diff | | --- | --- | --- | --- | | `ingestManager` | 386.2KB | 373.9KB | -12.3KB | | `telemetry` | 63.5KB | 50.1KB | -13.5KB | | `upgradeAssistant` | 74.5KB | 60.5KB | -14.0KB | | total | | | -39.7KB | ### The import paths look odd. Are they required? I agree and, no, they're not strictly required. If you'd like me to revert to the prior style just drop a comment and I'll undo them. The caveat is that the current style (in `master` & this PR) pulls in the entire `semver` library. In 7.x that added ~15K to the initial size. Some more details in the comments: #83020 (comment) ### Possible issues Moving 2 major versions. We're currently on 5.7 and the latest available is 7.3. * changelog says 5.x (our current) to 6.0 should be safe: https://github.com/npm/node-semver/blob/master/CHANGELOG.md#60 * There 6.x & 7.x changes all appear to be new features or bugfixes around the `includePrerelease` flag added in 5.6, but I'm not sure if those "fixes" will break existing code * https://github.com/npm/node-semver/blob/master/CHANGELOG.md#613 * https://github.com/npm/node-semver/blob/master/CHANGELOG.md#722 ### Stats / screenshots generated with `node scripts/build_kibana_platform_plugins.js --profile --focus=ingestManager` <details><summary><b>Ingest Manager in `master`</b>: imports entire `semver` lib, totals 40k+, only 1 large file (orange arc below)</summary> <img width="972" alt="Screen Shot 2020-11-09 at 6 50 23 PM" src="https://user-images.githubusercontent.com/57655/98666188-a50ac380-231a-11eb-9b8a-6ca784752714.png"> </details> <details><summary><b>Ingest Manager in PR after upgrade to 7</b>: still imports entire lib. file size *increased* to ~60k, but now individual files are imported (orange arcs below)</summary> <img width="825" alt="Screen Shot 2020-11-09 at 5 46 30 PM" src="https://user-images.githubusercontent.com/57655/98666355-e602d800-231a-11eb-803f-bc04beb4eaf1.png"> <img width="963" alt="Screen Shot 2020-11-09 at 5 47 06 PM" src="https://user-images.githubusercontent.com/57655/98666357-e69b6e80-231a-11eb-92d3-c66904f92c30.png"> </details> <details><summary><b>Ingest Manager in PR after changing `import`s:</b> total imported size down to ~20k. Can see individual imported files</summary> <img width="926" alt="Screen Shot 2020-11-10 at 6 10 23 AM" src="https://user-images.githubusercontent.com/57655/98667058-e64fa300-231b-11eb-9690-5e36ed6475e0.png"> <img width="895" alt="Screen Shot 2020-11-10 at 6 10 53 AM" src="https://user-images.githubusercontent.com/57655/98667059-e780d000-231b-11eb-8abf-98d8bdbcf061.png"> </details> ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
What's changed in this PR
Update to latest available
semver
:7.3.2
semver
5.x pulls in the entire library in one large file (~38k uncompressed / ~9k gz), when we might only use 1-2K.semver
7.0+ supports tree-shaking: https://github.com/npm/node-semver/blob/master/CHANGELOG.md#700Update paths to only import individual function(s) used instead of the entire library
public
&common
folders; notserver
. We could also updateserver
as well for consistency, but I skipped because the new import style is more verbose and the filesize didn't seem as important on the serverResults
The build stats show a 10K+ improvement for initial page bundles #83020 (comment)
ingestManager
telemetry
upgradeAssistant
The import paths look odd. Are they required?
I agree and, no, they're not strictly required. If you'd like me to revert to the prior style just drop a comment and I'll undo them.
The caveat is that the current style (in
master
& this PR) pulls in the entiresemver
library. In 7.x that added ~15K to the initial size. Some more details in the comments: #83020 (comment)Possible issues
Moving 2 major versions. We're currently on 5.7 and the latest available is 7.3.
includePrerelease
flag added in 5.6, but I'm not sure if those "fixes" will break existing codeStats / screenshots
generated with
node scripts/build_kibana_platform_plugins.js --profile --focus=ingestManager
Ingest Manager in `master`: imports entire `semver` lib, totals 40k+, only 1 large file (orange arc below)
Ingest Manager in PR after upgrade to 7: still imports entire lib. file size *increased* to ~60k, but now individual files are imported (orange arcs below)
Ingest Manager in PR after changing `import`s: total imported size down to ~20k. Can see individual imported files
Checklist