Do not serve legacy JS when serving a Kibana Platform application #61011

joshdover · 2020-03-23T22:54:34Z

Summary

Removing all legacy imports for src/core/public
Adding a new bundle configuration to @kbn/optimizer
Update the bootstrapping code to load only the core bundle when we're not rendering a legacy application
Removes the legacy core bundle

To do

Add tests for core bundle in @kbn/optimizer

Page load time improvements

Here are some stats on a production build comparing this branch to the latest snapshot from master.

	Before - Cold Start	After - Cold Start	Diff	Before - Cached	After - Cached	Diff
Number of requests	128	100	-22%	128	100	-22%
Data transferred	20mb	15.6mb	-22%	528kb	516kb	-2%
Total resources	103mb	82.2mb	-20%	103mb	82.2mb	-20%
Time	~12s	~8.5s	-30%	~8s	~7s	-12%

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

src/legacy/ui/ui_render/bootstrap/template.js.hbs

src/legacy/ui/ui_render/ui_render_mixin.js

joshdover · 2020-03-23T23:07:21Z

src/core/public/saved_objects/saved_objects_client.ts

+          window.location.assign(
+            this.http.basePath.prepend('/app/kibana#/error/action.auto_create_index')
+          );


TODO: test this case

🙈 on this if (isAutoCreateIndexError(error)) { block, but we need to keep the TODO to at least adapt the redirecting url when the page is migrated from legacy.

is there a test for this?

We can remove the auto create index error handling code and views. Probably out of scope for this PR, so I created #63894

elasticmachine · 2020-03-24T11:40:32Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet

Approach looks good to me. Now, is it working 😄 ?

src/core/public/entry_point.ts

src/core/public/legacy/legacy_service.ts

pgayvallet · 2020-03-25T14:03:39Z

src/core/public/saved_objects/saved_objects_client.ts

+          window.location.assign(
+            this.http.basePath.prepend('/app/kibana#/error/action.auto_create_index')
+          );


🙈 on this if (isAutoCreateIndexError(error)) { block, but we need to keep the TODO to at least adapt the redirecting url when the page is migrated from legacy.

src/legacy/ui/ui_render/bootstrap/template.js.hbs

x-pack/plugins/graph/public/application.ts

pgayvallet · 2020-04-09T09:11:11Z

@joshdover is this ready for review?

joshdover · 2020-04-09T16:27:11Z

@joshdover is this ready for review?

I'm planning to add a more tests for the @kbn/optimizer changes, other than that it should be ready.

The other to do is to create a dedicated core bundle that is separate from the core entry point so that more code sharing can be leveraged with the plugin and legacy bundles. However, this can be done in a future PR.

packages/kbn-optimizer/src/optimizer/get_bundles.ts

src/legacy/ui/ui_render/bootstrap/template.js.hbs

joshdover · 2020-04-20T17:56:29Z

@cchaos I actually decided to rollback these SCSS changes. I was originally trying to avoid loading some legacy CSS but it turns out there's more there we need right now and cleaning that up feels out of scope for this PR.

For now we will continue to load most of the legacy CSS (still excluding legacy plugin-specific CSS) so that everything still works. We can focus on this more deliberately in #58369.

cchaos · 2020-04-20T18:10:12Z

Got it thanks 👍. It would be great if we could eventually have some guidelines around how SASS/CSS is compiled in some form of documentation like the CONTRIBUTING doc. I think this is the most misunderstood part of writing plugins. We have "How to write SASS", but not how to import it into the file system.

joshdover · 2020-04-20T20:38:40Z

@cchaos Scratch that, it was necessary to move. I addressed all of your comments and pushed again with those CSS changes.

spalger · 2020-04-20T23:18:37Z

packages/kbn-optimizer/src/worker/webpack.config.ts

@@ -162,7 +162,7 @@ export function getWebpackConfig(bundle: Bundle, worker: WorkerConfig) {
          ],
        },
        {
-          test: /\.scss$/,
+          test: /\.(scss|css)$/,


We shouldn't be passing css to the sass-loader, should we? If the file should be parsed as sass I think we should change the extension to scss and not automatically parse it as scss.

Ah yes, I added this in my first POC iterations and forgot to remove.

src/core/public/entry_point.ts

mshustov · 2020-04-21T09:39:53Z

src/legacy/ui/ui_render/ui_render_mixin.js

-                : `${basePath}/${path.publicPath}`
-            )
-            .reverse(),
+          ...(isCore


can be merged with https://github.com/elastic/kibana/pull/61011/files#diff-ce5eebb41b6315c38446e8a68a4ca0aeR108

Since the order of CSS files affects the rendered styles, I decided to preserve the current ordering. I think we want to make sure that DLL styles are loaded first so that all subsequent sheets can override them.

It's just too hard to accidentally break things with CSS so I'm erring on the side caution here.

src/legacy/ui/ui_render/ui_render_mixin.js

mshustov · 2020-04-21T09:43:21Z

src/legacy/ui/ui_render/bootstrap/template.js.hbs

-        '{{regularBundlePath}}/kbn-ui-shared-deps/{{sharedJsFilename}}',
-        '{{dllBundlePath}}/vendors_runtime.bundle.dll.js',
-        {{#each dllJsChunks}}
+        {{#each jsDependencies}}


❤️ this diff

mshustov · 2020-04-21T12:20:54Z

packages/kbn-optimizer/src/common/bundle.ts

@@ -23,7 +23,7 @@ import { BundleCache } from './bundle_cache';
 import { UnknownVals } from './ts_helpers';
 import { includes, ascending, entriesToObject } from './array_helpers';

-const VALID_BUNDLE_TYPES = ['plugin' as const];
+const VALID_BUNDLE_TYPES = ['plugin' as const, 'entry' as const];


The inclusion of plugins in those bundles has caused a number of issues over the years and it seems there needs not be a whole "platform" loaded to render those two pages.

Not sure it's feasible. That would add a limitation that security plugin cannot use other plugins API (licensing, for example) on login / logout pages. Also, it's already working with bootstrapping all the KP plugins.

cchaos

👍 Thanks for making those SASS changes!

cchaos · 2020-04-21T14:04:07Z

src/core/public/index.scss

-
-// Mixins provide generic code expansion through helpers
-@import '@elastic/eui/src/global_styling/mixins/index';
+// This file is built by both the legacy and KP build systems so we need to


❤️ Thanks for this comment!

implement `includeCoreBundle: boolean` config

packages/kbn-optimizer/src/__fixtures__/mock_repo/src/core/public/entry_point.ts

spalger

Optimizer side of things LGTM

joshdover · 2020-04-21T23:24:51Z

@flash1293 would you mind taking a look at the graph changes? I removed an unused scss import and added a manual import for angular-route that was inadvertently imported by core before.

joshdover · 2020-04-22T15:22:07Z

@elasticmachine merge upstream

flash1293

Tested Graph changes in Chrome and everything works as expected, LGTM 👍 I will take care of inlining the wrapper styles in a separate PR.

kibanamachine · 2020-04-22T17:05:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6f648b1

History

💚 Build #42217 succeeded 237e2c5
💚 Build #42025 succeeded 329c044
💔 Build #41987 failed 9b6d30a
💔 Build #41768 failed af039ee
💔 Build #41754 failed 32c572a

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

…tic#61011)

joshdover mentioned this pull request Mar 23, 2020

Serve a slimmer core-only bundle for Kibana Platform applications #60580

Closed

2 tasks

joshdover commented Mar 23, 2020

View reviewed changes

src/legacy/ui/ui_render/bootstrap/template.js.hbs Outdated Show resolved Hide resolved

joshdover commented Mar 23, 2020

View reviewed changes

src/legacy/ui/ui_render/ui_render_mixin.js Outdated Show resolved Hide resolved

joshdover commented Mar 23, 2020

View reviewed changes

streamich added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Mar 24, 2020

pgayvallet linked an issue Mar 24, 2020 that may be closed by this pull request

Serve a slimmer core-only bundle for Kibana Platform applications #60580

Closed

2 tasks

pgayvallet reviewed Mar 25, 2020

View reviewed changes

joshdover force-pushed the np/slim-bundle branch 2 times, most recently from 80fe38d to 3f17e0a Compare April 7, 2020 22:43

pgayvallet reviewed Apr 8, 2020

View reviewed changes

x-pack/plugins/graph/public/application.ts Show resolved Hide resolved

joshdover force-pushed the np/slim-bundle branch from 98c767e to 77c47ef Compare April 8, 2020 17:29

joshdover added 5 commits April 9, 2020 14:16

Remove legacy imports from core/public

7af9555

Add core bundle to @kbn/optimizer

76f0ee8

Add core-only bundle loading strategy

a37876f

Update kbn-optimizer tests

4aba2c5

Rename standalone to entry, fix bootstrap

ef11745

joshdover force-pushed the np/slim-bundle branch from 77c47ef to ef11745 Compare April 9, 2020 20:58

joshdover changed the title ~~[Draft] Serve less JS when serving a new platform application~~ Serve less JS when serving a new platform application Apr 9, 2020

joshdover marked this pull request as ready for review April 9, 2020 21:18

joshdover requested review from a team as code owners April 9, 2020 21:18

joshdover added release_note:skip Skip the PR/issue when compiling release notes v7.8.0 labels Apr 9, 2020

joshdover added this to Pending Review in kibana-core [DEPRECATED] via automation Apr 9, 2020

joshdover commented Apr 9, 2020

View reviewed changes

packages/kbn-optimizer/src/optimizer/get_bundles.ts Outdated Show resolved Hide resolved

joshdover commented Apr 9, 2020

View reviewed changes

src/legacy/ui/ui_render/bootstrap/template.js.hbs Show resolved Hide resolved

joshdover changed the title ~~Serve less JS when serving a new platform application~~ Do not serve legacy JS when serving a Kibana Platform application Apr 9, 2020

Merge remote-tracking branch 'upstream/master' into np/slim-bundle

05ba7b1

joshdover added 2 commits April 20, 2020 13:23

Revert colon in plugin URL

9b6d30a

Move legacy CSS again

5d893d6

joshdover requested review from cchaos and spalger April 20, 2020 20:49

joshdover mentioned this pull request Apr 20, 2020

Improve plugin bundle load time #55241

Closed

joshdover and others added 2 commits April 20, 2020 16:14

Fix mixins import

329c044

implement includeCoreBundle: boolean config

fb44467

spalger reviewed Apr 20, 2020

View reviewed changes

mshustov reviewed Apr 21, 2020

View reviewed changes

cchaos approved these changes Apr 21, 2020

View reviewed changes

Merge pull request #30 from spalger/pr/61011

1fd2e06

implement `includeCoreBundle: boolean` config

spalger reviewed Apr 21, 2020

View reviewed changes

packages/kbn-optimizer/src/__fixtures__/mock_repo/src/core/public/entry_point.ts Outdated Show resolved Hide resolved

PR comments

237e2c5

pgayvallet mentioned this pull request Apr 21, 2020

Remove SO autocreateindex error and error page #64037

Merged

2 tasks

spalger approved these changes Apr 21, 2020

View reviewed changes

Merge branch 'master' into np/slim-bundle

6f648b1

flash1293 approved these changes Apr 22, 2020

View reviewed changes

joshdover merged commit fc1f4f2 into elastic:master Apr 22, 2020

kibana-core [DEPRECATED] automation moved this from Pending Review to Done (7.8) Apr 22, 2020

joshdover deleted the np/slim-bundle branch April 22, 2020 17:14

joshdover mentioned this pull request Apr 22, 2020

[7.x] Do not serve legacy JS when serving a Kibana Platform applicat… (#61011) #64218

Merged

joshdover added a commit to joshdover/kibana that referenced this pull request Apr 22, 2020

Do not serve legacy JS when serving a Kibana Platform applicat… (elas…

5cfb56e

…tic#61011)

legrego mentioned this pull request Apr 28, 2020

Import space selector styles #64656

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not serve legacy JS when serving a Kibana Platform application #61011

Do not serve legacy JS when serving a Kibana Platform application #61011

joshdover commented Mar 23, 2020 •

edited

Loading

joshdover Mar 23, 2020

pgayvallet Mar 25, 2020

mshustov Apr 14, 2020

rudolf Apr 17, 2020

elasticmachine commented Mar 24, 2020

pgayvallet left a comment

pgayvallet Mar 25, 2020

pgayvallet commented Apr 9, 2020

joshdover commented Apr 9, 2020

joshdover commented Apr 20, 2020

cchaos commented Apr 20, 2020

joshdover commented Apr 20, 2020

spalger Apr 20, 2020 •

edited

Loading

joshdover Apr 21, 2020

mshustov Apr 21, 2020

joshdover Apr 21, 2020

mshustov Apr 21, 2020

mshustov Apr 21, 2020

cchaos left a comment

cchaos Apr 21, 2020

spalger left a comment

joshdover commented Apr 21, 2020

joshdover commented Apr 22, 2020

flash1293 left a comment

kibanamachine commented Apr 22, 2020

Do not serve legacy JS when serving a Kibana Platform application #61011

Do not serve legacy JS when serving a Kibana Platform application #61011

Conversation

joshdover commented Mar 23, 2020 • edited Loading

Summary

Page load time improvements

Checklist

For maintainers

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 24, 2020

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Apr 9, 2020

joshdover commented Apr 9, 2020

joshdover commented Apr 20, 2020

cchaos commented Apr 20, 2020

joshdover commented Apr 20, 2020

spalger Apr 20, 2020 • edited Loading

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

cchaos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger left a comment

Choose a reason for hiding this comment

joshdover commented Apr 21, 2020

joshdover commented Apr 22, 2020

flash1293 left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 22, 2020

💚 Build Succeeded

History

joshdover commented Mar 23, 2020 •

edited

Loading

spalger Apr 20, 2020 •

edited

Loading