-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Remove legacy optimizer #73154
Remove legacy optimizer #73154
Conversation
c886236
to
5b6c5c6
Compare
21ecf55
to
7c820df
Compare
f132f77
to
25ca57a
Compare
3567e28
to
b058ea3
Compare
acbaadd
to
afa73c8
Compare
@elasticmachine merge upstream |
|
f7be502
to
12cc326
Compare
12cc326
to
8fbc20b
Compare
@elasticmachine merge upstream |
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 good. Checked without base-path and correctly see success/error messaging. Checked that editing in various themes trigger, and that editing the css directly works.
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.
Code review only of Kibana App owned files. Code changes LGTM
@elasticmachine merge upstream |
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 PR turned out to be less scary than I expected 😄 I actually have very little to comment on, most of this was just removing.
I think we can remove the /src/legacy/server/sass/ @elastic/kibana-operations
line from our CODEOWNERS
Do you agree that all of the bundle serving logic in src/optimize
should be moved to src/core/server
when we start removing src/legacy/server
? Definitely out of scope for this PR, just wanted to gut check my assumptions here.
@@ -55,6 +55,11 @@ export async function bootstrap({ | |||
onRootShutdown('Kibana REPL mode can only be run in development mode.'); | |||
} | |||
|
|||
if (cliArgs.optimize) { | |||
// --optimize is deprecated and does nothing now, avoid starting up and just shutdown |
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.
Should we log a warning?
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.
I tried, but the logger is in buffering mode here and I don't think we should write something to stdout or something without considering the other flags... I figured the --help
and release notes are sufficient to warn people that they don't need to call this. In 8.0 we'll remove the flag and then it will stop working, I don't think we need to figure out how to make it log. #73154 (comment)
@@ -118,7 +118,6 @@ | |||
}, | |||
"dependencies": { | |||
"@babel/core": "^7.11.1", |
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.
Are we still using babel in production? I thought we pre-babel'd everything during the build?
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.
We still babel-register, but I plan to remove that soon, once the plugin-helpers take care of babel-ifying things at build time.
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
async chunks size
page load bundle size
History
To update your PR or re-run it, just comment with: |
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.
I tested Canvas, Storybook (dev and build) and the Shareable Runtime (dev and build) and all is working! Great job, @spalger
Co-authored-by: spalger <spalger@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # docs/setup/install/deb.asciidoc # docs/setup/install/rpm.asciidoc # docs/setup/install/targz.asciidoc # docs/setup/install/windows.asciidoc # package.json # packages/kbn-pm/dist/index.js # packages/kbn-test/src/functional_tests/lib/paths.js # src/core/server/config/deprecation/core_deprecations.ts # src/legacy/server/config/schema.js # src/optimize/base_optimizer.js # test/scripts/jenkins_xpack.sh # x-pack/legacy/plugins/xpack_main/index.js # yarn.lock
* upstream/master: (45 commits) [Metrics UI] Fix inventory footer misalignment (elastic#74707) Remove legacy optimizer (elastic#73154) Update design-specific GH code-owners (elastic#74877) skip test Reporting paginates content elastic#74922 [Metrics UI] Add Jest tests for alert previews (elastic#74890) Fixed tooltip (elastic#74074) [Ingest Pipelines] Processor forms for processors A-D (elastic#72849) [Observability] change ingest manager link (elastic#74928) [Task manager] Prevents edge case where already running tasks are reschedule every polling interval (elastic#74606) [Security Solution] Fix the status of timelines' bulk actions (elastic#74560) Data plugin: Suggested enhance pattern (elastic#74505) Use jest.useFakeTimers instead of hard coded timeout for tooltip tests. (elastic#74642) [Security Solution][lists] Adds tests for exception lists and items part 2 (elastic#74815) [Security Solution][Resolver] fix presentation role on edgeline (elastic#74869) [Security Solution][Detections] Refactor ML calls for newest ML permissions (elastic#74582) [bin/kibana-plugin] support KP plugins instead (elastic#74604) Reduce number of indexed fields in index pattern saved object (elastic#74817) [reporting] Pass along generic parameters in high-order route handler (elastic#74892) Migrated last pieces of legacy fixture code (elastic#74470) Empty index patterns page re-design (elastic#68819) ...
This PR is good. |
* master: (23 commits) Adding API test for custom link transaction example (elastic#74238) [Uptime] Singular alert (elastic#74659) Revert "attempt excluding a codeowners directory" (elastic#75023) [Metrics UI] Remove TSVB dependency from Metrics Explorer APIs (elastic#74804) Remove degraded state from ES status service (elastic#75007) [Reporting/Functional] unskip pagination test (elastic#74973) [Resolver] Stale query string values are removed when resolver's component instance ID changes. (elastic#74979) Add public url to Workplace Search plugin (elastic#74991) [Reporting] Update more Server Types for TaskManager (elastic#74915) [I18n] verify select icu-message options are in english (elastic#74963) Make data.search.aggs available on the server. (elastic#74472) [Security Solution][Resolver] Graph Control Tests and Update Simulator Selectors (elastic#74680) attempt excluding a codeowners directory [ML] DF Analytics: allow failed job to be stopped by force via the UI (elastic#74710) Add kibana-core-ui-designers team (elastic#74970) [Metrics UI] Fix inventory footer misalignment (elastic#74707) Remove legacy optimizer (elastic#73154) Update design-specific GH code-owners (elastic#74877) skip test Reporting paginates content elastic#74922 [Metrics UI] Add Jest tests for alert previews (elastic#74890) ...
Fixes #7322
Fixes #58369
Closes #19633
Closes #6495
Closes #27274
Closes #45409
Closes #53160
The legacy optimizer is no longer required so this PR removes it along with a handful of related components. I tried to break this out into separate PRs, but the extra cost of all that review was weighing heavy on the effort to just get this removed so I ditched that approach and moved forward with a pretty colossal deletion PR.
Mostly complete change list:
--optimize
CLI flag, we now log and exit as soon as we start with this flag.optimize.*
config options have been deprecated with log messages about removaloptimize.viewCaching
was used by an unused view rendering layer in the legacy server, which I removed so I could remove this config valueui/*
imports in KP scss was removed, remaining usage was converted to use~
css-loader syntax to pull from assets insrc/core/public/core_app
The@kbn/optimizer
max workers count is nowMath.max(CPU_COUNT - 1, 3)
src/legacy/ui/public/styles
tosrc/core/public/core_app/styles
src/legacy/ui/public
were converted from less to css and stored insrc/core/server/core_app/assets
src/core/public/core_app/images
--no-optimizer
can now be passed toyarn start
if people want to run the@kbn/optimizer
separately (used to be--optimize.enabled
, no deprecation since this is likely never used and dev only)/bundles/app/{id}/bootstrap.js
to just/bootstrap.js
as it's not a bundle and it's not app specificsyncToFile
option was removed as this was only necessary to support--optimize
flagUiBundles
removedbundleRoute
is now just responsible for cache/etag logic, good candidate for replacement with more standard logicsrc/core/server/core_app/assets/legacy_light_theme.css
rather than building sass and importing plugin light themes from build_assets or ui/public/stylesStats:
This produces a modest 3MB reduction in the side of the default distributable, 2646 fewer files. Server startup time doesn't seem to be impacted at all by this change, which is generally expected since we've moved basically all the files from legacy to the Kibana Platform.
Holy change set!:
FYI, 23310 of the 24043 additions (97%) were caused by the
@kbn/pm
dist as well as committing the compiled versions ofsrc/core/server/core_app/assets/legacy_light_theme.css
andsrc/core/server/core_app/assets/legacy_dark_theme.css
. That leaves 733 lines actually changed. Please don't be overwhelmed by the described changes :)Release note:
Kibana no longer needs to optimize plugins for use in the browser when a plugin is installed. This means the
--optimize
flag is now deprecated and does nothing now. It will be removed in 8.0.