Migrate the optimizer mixin to core #94272

pgayvallet · 2021-03-10T09:44:57Z

Summary

Fix #77020

Migrate the legacy optimize mixin (/src/optimize) that is serving our webpack bundles to core.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…-optimizer-mixin

pgayvallet

Self-review

src/core/server/core_app/bundle_routes/file_hash_cache.ts

pgayvallet · 2021-03-11T10:11:35Z

src/core/server/core_app/bundle_routes/fs.ts

+// can't use fs/promises when working with streams using file descriptors
+// see https://github.com/nodejs/node/issues/35862
+
+import Fs from 'fs';
+import { promisify } from 'util';


Spent one hour trying to properly use fs/promises with createReadStream, then I had to fall back to promisify as it was in legacy.

TIL: promisified fs are not the exact same as the new APIs provided by fs/promises.

pgayvallet · 2021-03-11T10:13:55Z

src/core/server/core_app/bundle_routes/register_bundle_routes.ts

+  [...uiPlugins.internal.entries()].forEach(([id, { publicTargetDir }]) => {
+    registerRouteForBundle(router, {
+      publicPath: `${serverBasePath}/${buildNum}/bundles/plugin/${id}/`,
+      routePath: `/${buildNum}/bundles/plugin/${id}/`,
+      bundlesPath: publicTargetDir,
+      fileHashCache,
+      isDist,
+    });
+  });


We could probably optimize this a little to have a single route registered to serve all plugins bundle. I didn't bother, as it created divergence in the underlying registerRouteForBundle between plugins and non-plugins bundles, and just introduced more complexity, which isn't worth it ihmo.

pgayvallet · 2021-03-11T10:15:22Z

src/core/server/core_app/bundle_routes/select_compressed_file.ts

+export async function selectCompressedFile(acceptEncodingHeader: string | undefined, path: string) {
+  let fd: number | undefined;
+  let fileEncoding: 'gzip' | 'br' | undefined;
+  const ext = extname(path);
+
+  const supportedEncodings = Accept.encodings(acceptEncodingHeader, ['br', 'gzip']);
+
+  // do not bother trying to look compressed versions for anything else than js or css files
+  if (ext === '.js' || ext === '.css') {


I did not unit test this specific file, because we would just be mocking the filesystem APIs and the testing process would be tedious for little value. This is covered both by integration and FTR tests anyway.

pgayvallet · 2021-03-11T10:16:34Z

src/core/server/core_app/bundle_routes/utils.ts

-
-  const promise = Rx.merge(
+  return Rx.merge(
    Rx.fromEvent<Buffer>(read, 'data'),
    Rx.fromEvent<Error>(read, 'error').pipe(
      map((error) => {


Not sure of the value of using an observable approach to generate the hash tbh, but it's working, so I kept the implementation as it was in legacy.

Yeah I'm sure this creates 100s of RxJs objects, but guess fine for now.

pgayvallet · 2021-03-11T10:19:30Z

src/core/server/core_app/core_app.ts

-  setup(coreSetup: InternalCoreSetup) {
+  setup(coreSetup: InternalCoreSetup, uiPlugins: UiPlugins) {


We need the uiPlugins definition to register the bundle routes, so I had to add it to the CoreApp.setup parameters, as it's not available from the setup contract.

pgayvallet · 2021-03-11T10:20:25Z

src/legacy/server/kbn_server.js

-        // setup routes that serve the @kbn/optimizer output
-        optimizeMixin
+        loggingMixin


Getting closer.

elasticmachine · 2021-03-11T10:27:43Z

Pinging @elastic/kibana-core (Team:Core)

…-optimizer-mixin

spalger

LGTM

joshdover

Looks great. Tested locally and everything seemed to work well 👍

joshdover · 2021-03-16T17:03:55Z

src/core/server/core_app/integration_tests/__fixtures__/plugin/foo/gzip_chunk.js

@@ -6,4 +6,4 @@
 * Side Public License, v 1.
 */

-export { optimizeMixin } from './optimize_mixin';
+module.exports = 'GZIP-CHUNK';


Git thinks this is a rename, sigh 🤦

joshdover · 2021-03-16T17:05:20Z

src/core/server/core_app/bundle_routes/utils.ts

-
-  const promise = Rx.merge(
+  return Rx.merge(
    Rx.fromEvent<Buffer>(read, 'data'),
    Rx.fromEvent<Error>(read, 'error').pipe(
      map((error) => {


Yeah I'm sure this creates 100s of RxJs objects, but guess fine for now.

…-optimizer-mixin

kibanamachine · 2021-03-17T08:55:56Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #113002 succeeded 1291e3b
💚 Build #112702 succeeded 85f2337
💔 Build #112684 failed 5201829
💔 Build #112403 failed 5421bfb

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* migrate optimizer mixin to core apps * fix core_app tests * add integration tests, extract selectCompressedFile * add CoreApp unit test * more unit tests * unit tests for bundle_route * more unit tests * remove /src/optimize/ from codeowners * fix case * NIT # Conflicts: # .github/CODEOWNERS

…-action * 'master' of github.com:elastic/kibana: (44 commits) Migrate the optimizer mixin to core (#94272) Replace EuiCodeBlock with JsonCodeEditor in DiscoverGrid (#92442) [ML] Anomaly Detection: Migrate validation messages links to use docLinks. (#94568) [Lists][Exceptions] - Adding basic linting, i18n and storybook support (#94772) [Fleet] Add test/fix for invalid/missing ids in bulk agent reassign (#94632) [Security Solution] [Cases] Move create page components and dependencies to Cases (#94444) [ML] Data Frame Analytics accessibility tests: fix flaky outlier creation test (#94735) [Security Solutions] Remove commented out old linter rules (#94753) [App Search] Role mappings migration part 2 (#94461) [CI] Update Backport action inputs to match updated ones (#94721) [chore] Remove the infra team from CODEOWNERS (#94740) [Connectors UI] Make UI use new connector APIs (#94180) [ML] Use indices options in anomaly detection job wizards (#91830) Remove `string` as a valid registry var type value (#94174) [Alerts] Replaces legacy es client with the ElasticsearchClient for alerts and triggers_actions_ui plugins. (#93364) [Reporting-CSV Export] Re-write CSV Export using SearchSource (#88303) chore(NA): upgrade bazel rules nodejs to v3.2.2 (#94726) [APM] Settings: Update layout and update/add descriptions (#94398) skip flaky suite (#94666) [XY axis] Integrates legend color picker with the eui palette (#90589) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/constants.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/configuration_context.tsx # x-pack/plugins/index_lifecycle_management/public/shared_imports.ts

* migrate optimizer mixin to core apps * fix core_app tests * add integration tests, extract selectCompressedFile * add CoreApp unit test * more unit tests * unit tests for bundle_route * more unit tests * remove /src/optimize/ from codeowners * fix case * NIT # Conflicts: # .github/CODEOWNERS

pgayvallet added 2 commits March 10, 2021 10:39

migrate optimizer mixin to core apps

8f51fd6

Merge remote-tracking branch 'upstream/master' into kbn-77020-migrate…

5421bfb

…-optimizer-mixin

pgayvallet changed the title ~~Kbn 77020 migrate optimizer mixin~~ Migrate the optimizer mixin to core Mar 10, 2021

pgayvallet added Feature:Legacy Removal Issues related to removing legacy Kibana release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.13.0 chore labels Mar 10, 2021

pgayvallet added 11 commits March 10, 2021 20:30

Merge remote-tracking branch 'upstream/master' into kbn-77020-migrate…

c6be28c

…-optimizer-mixin

fix core_app tests

0ed7f36

add integration tests, extract selectCompressedFile

86b8f47

Merge remote-tracking branch 'upstream/master' into kbn-77020-migrate…

5201829

…-optimizer-mixin

add CoreApp unit test

6411d69

more unit tests

1e83e00

unit tests for bundle_route

5f396f9

more unit tests

fe10e1c

remove /src/optimize/ from codeowners

5abec62

fix case

93e54ec

NIT

85f2337

pgayvallet commented Mar 11, 2021

View reviewed changes

pgayvallet marked this pull request as ready for review March 11, 2021 10:27

pgayvallet requested review from a team as code owners March 11, 2021 10:27

Merge remote-tracking branch 'upstream/master' into kbn-77020-migrate…

1291e3b

…-optimizer-mixin

spalger approved these changes Mar 16, 2021

View reviewed changes

joshdover approved these changes Mar 16, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-77020-migrate…

b698df9

…-optimizer-mixin

pgayvallet merged commit 25e586a into elastic:master Mar 17, 2021

pgayvallet mentioned this pull request Mar 17, 2021

[7.x] Migrate the optimizer mixin to core (#94272) #94782

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate the optimizer mixin to core #94272

Migrate the optimizer mixin to core #94272

pgayvallet commented Mar 10, 2021 •

edited

Loading

pgayvallet left a comment

pgayvallet Mar 11, 2021

pgayvallet Mar 11, 2021

pgayvallet Mar 11, 2021

pgayvallet Mar 11, 2021

joshdover Mar 16, 2021

pgayvallet Mar 11, 2021

pgayvallet Mar 11, 2021

elasticmachine commented Mar 11, 2021

spalger left a comment

joshdover left a comment

joshdover Mar 16, 2021

joshdover Mar 16, 2021

kibanamachine commented Mar 17, 2021

		setup(coreSetup: InternalCoreSetup) {
		setup(coreSetup: InternalCoreSetup, uiPlugins: UiPlugins) {

Migrate the optimizer mixin to core #94272

Migrate the optimizer mixin to core #94272

Conversation

pgayvallet commented Mar 10, 2021 • edited Loading

Summary

Checklist

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 11, 2021

spalger left a comment

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Mar 17, 2021

💚 Build Succeeded

Metrics [docs]

History

pgayvallet commented Mar 10, 2021 •

edited

Loading