Skip to content
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

document code splitting for client code #62593

Merged
merged 2 commits into from
Apr 14, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 61 additions & 0 deletions src/core/MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
- [7. Switch to new platform services](#7-switch-to-new-platform-services)
- [8. Migrate to the new plugin system](#8-migrate-to-the-new-plugin-system)
- [Bonus: Tips for complex migration scenarios](#bonus-tips-for-complex-migration-scenarios)
- [Keep Kibana fast](#keep-kibana-fast)
- [Frequently asked questions](#frequently-asked-questions)
- [Is migrating a plugin an all-or-nothing thing?](#is-migrating-a-plugin-an-all-or-nothing-thing)
- [Do plugins need to be converted to TypeScript?](#do-plugins-need-to-be-converted-to-typescript)
Expand Down Expand Up @@ -933,6 +934,66 @@ For a few plugins, some of these steps (such as angular removal) could be a mont

One convention that is useful for this is creating a dedicated `public/np_ready` directory to house the code that is ready to migrate, and gradually move more and more code into it until the rest of your plugin is essentially empty. At that point, you'll be able to copy your `index.ts`, `plugin.ts`, and the contents of `./np_ready` over into your plugin in the new platform, leaving your legacy shim behind. This carries the added benefit of providing a way for us to introduce helpful tooling in the future, such as [custom eslint rules](https://github.com/elastic/kibana/pull/40537), which could be run against that specific directory to ensure your code is ready to migrate.

## Keep Kibana fast
**tl;dr**: Load as much code lazily as possible.
Everyone loves snappy applications with responsive UI and hates spinners. Users deserve the best user experiences regardless of whether they run Kibana locally or in the cloud, regardless of their hardware & environment.
Comment on lines +937 to +939
Copy link
Contributor

@pgayvallet pgayvallet Apr 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's outside of code splitting (well, of splitting plugin bundles), but another thing that is currently increasing our bundles size drastically are static imports that can be done between plugins (at least until #62390 is merged)

See 43a9d45 for example, where even with async mount method for the route mount function, all dependencies were kept because of a static import from another plugin that was using them (in addition to shipping the whole dependency plugin).

Not sure if it belongs there or more in our CONVENTION, but we should probably also declare that static exports from plugins should be avoided as much as possible and exposed from the plugins contracts instead. This ensure both that plugins will not import more that they should from other plugins and also reduce the overall initial load, as the 'static' code will only be shipped in the providing plugin code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but we should probably also declare that static exports from plugins should be avoided as much as possible and exposed from the plugins contracts instead

I do agree it's a problem: a plugin importing anything from the data plugin will get additional (mostly unnecessary) 1.4Mb of code.
But I'd argue we shouldn't ban of static imports usage. Using static imports is a valid use-case for sharing stateless code. If we forced plugins to use plugins contracts only, it would worsen DX because it increases the number of dependencies passed down the code. I'd rather say the current situation is a technical limitation, that shouldn't dictate plugin authors how to write their code and platform & operation teams have to fix it eventually.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking a little about it you are right imho, this is only driven by a technical limitation that should be addressed, and is not a bad practice on itself.

There are 2 main aspects of the perceived speed of an application: loading time and responsiveness to user actions.
New platform loads and bootstraps **all** the plugins whenever a user lands on any page. It means that adding every new application affects overall **loading performance** in the new platform, as plugin code is loaded **eagerly** to initialize the plugin and provide plugin API to dependent plugins.
However, it's usually not necessary that the whole plugin code should be loaded and initialized at once. The plugin could keep on loading code covering API functionality on Kibana bootstrap but load UI related code lazily on-demand, when an application page or management section is mounted.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe insist on the fact that even if UI components size may be minimal, excluding/splitting them from the initial bundle also excludes all UI-related dependencies such as react and react-dom. UI related deps are almost always > 2.5mb uncompressed from what I saw.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would maybe insist on the fact that even if UI components size may be minimal

do you mean something like always prefer to load UI components lazily, even if UI components size seems to be negligible, because it requires loading of some rendering tools?

initial bundle also excludes all UI-related dependencies such as react and react-dom

react & react-dom are already excluded from a bundle as provided via kbn-ui-shared-deps.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not the best wording, but the idea would be always prefer to require UI root components lazily when possible (such as in mount handlers). Even if their size may seem negligible, it's very likely that they are using some heavy-weight libraries that will also be removed from the initial plugin bundle therefor reducing its size by a significant amount

react & react-dom are already excluded from a bundle as provided via kbn-ui-shared-deps.

From #62263 (comment) I think kbn-ui-shared-deps is used only by legacy plugins? Even if not, there seems to be other heavy-weight dependencies that are imported mostly/only from UI components as cutting them the the main bundle reduces its size by a lot.

Always prefer to require UI root components lazily when possible (such as in mount handlers). Even if their size may seem negligible, they are likely using some heavy-weight libraries that will also be removed from the initial plugin bundle, therefore, reducing its size by a significant amount.

```typescript
import { Plugin, CoreSetup, AppMountParameters } from 'src/core/public';
export class MyPlugin implements Plugin<MyPluginSetup> {
setup(core: CoreSetup, plugins: SetupDeps){
core.application.register({
id: 'app',
title: 'My app',
async mount(params: AppMountParameters) {
const { mountApp } = await import('./app/mount_app');
return mountApp(await core.getStartServices(), params);
},
});
plugins.management.sections.getSection('another').registerApp({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: this returns Section | undefined so the example would need a getSection('another')!. to be totally exact, but we probably don't care for the role of the example.

id: 'app',
title: 'My app',
order: 1,
async mount(params) {
const { mountManagementSection } = await import('./app/mount_management_section');
return mountManagementSection(coreSetup, params);
},
})
return {
doSomething(){}
}
}
}
```

#### How to understand how big the bundle size of my plugin is?
New platform plugins are distributed as a pre-built with `@kbn/optimizer` package artifacts. It allows us to get rid of the shipping of `optimizer` in the distributable version of Kibana.
Every NP plugin artifact contains all plugin dependencies required to run the plugin, except some stateful dependencies shared across plugin bundles via `@kbn/ui-shared-deps`.
It means that NP plugin artifacts tend to have a bigger size than the legacy platform version.
To understand the current size of your plugin artifact, run `@kbn/optimizer` as
```bash
node scripts/build_kibana_platform_plugins.js --dist --no-examples
```
and check the output in the `target` sub-folder of your plugin folder
```bash
ls -lh plugins/my_plugin/target/public/
# output
# an async chunk loaded on demand
... 262K 0.plugin.js
# eagerly loaded chunk
... 50K my_plugin.plugin.js
```
you might see at least one js bundle - `my_plugin.plugin.js`. This is the only artifact loaded by the platform during bootstrap in the browser. The rule of thumb is to keep its size as small as possible.
Other lazily loaded parts of your plugin present in the same folder as separate chunks under `{number}.plugin.js` names.
If you want to investigate what your plugin bundle consists of you need to run `@kbn/optimizer` with `--profile` flag to get generated [webpack stats file](https://webpack.js.org/api/stats/).
Many OSS tools are allowing you to analyze generated stats file
- [an official tool](http://webpack.github.io/analyse/#modules) from webpack authors
- [webpack-visualizer](https://chrisbateman.github.io/webpack-visualizer/)
Comment on lines +994 to +995
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally have been using webpack-bundle-analyzer https://www.npmjs.com/package/webpack-bundle-analyzer on it's CLI version (webpack-bundle-analyzer bundle/output/path/stats.json). not sure if we want to be exhaustive here though.


## Frequently asked questions

### Is migrating a plugin an all-or-nothing thing?
Expand Down