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

[kbn/optimizer] prevent duplication of bundles #64843

Closed
spalger opened this issue Apr 29, 2020 · 6 comments · Fixed by #68986
Closed

[kbn/optimizer] prevent duplication of bundles #64843

spalger opened this issue Apr 29, 2020 · 6 comments · Fixed by #68986
Assignees
Labels
enhancement New value added to drive a business result Team:Operations Team label for Operations Team

Comments

@spalger
Copy link
Contributor

spalger commented Apr 29, 2020

problem

A key part of the way plugins are built for the Kibana Platform is that they are independent and created using their own webpack instances, avoiding the need for rebuilding as plugins come and go.

A large amount of code sharing between plugins has lead to these independent bundles getting copied into each other as plugins import from one another, causing page load performance issues that we had to scramble to fix for 7.7. Now that the issues are temporarily fixed, we need to either disallow or better support the amount of cross plugin imports we are seeing.

We have discussed the option of requiring that all static code shared between plugins be shared via the plugin contracts instead of via imports (as intended during the design of the Kibana Platform) but that is not very idiomatic, so we're going to try to find a way to better support what developers naturally expect to work.

temporary solution

In order to quickly mitigate the most common issues caused by cross-plugin importing we manually chose a list of plugins that were often imported and fairly large, and modified the way builds are produced to share the bundles with all dependents:

  1. The plugin bundles write the entire namespace of the public/index.ts file in their bundle to window.__kbnBundles__['plugin/${id}']
  2. Any import statement resolving to public/index.ts in one of the shared plugins was rewritten to access the namespace via the __kbnBundles__ global variable via webpack externals.

Doing this allowed us to greatly reduce the size of the bundles in the short term, but we need a more complete solution going forward.

proposed solution

The @kbn/optimizer will support plugins importing from the /public and /common roots of other plugins by automatically setting up namespaces similar to how we did in our temporary solution. Any plugin that has a relative import which resolves into another plugin's directory (ie. src/plugins/data) but does not resolve to the root of the /public or /commmon directories of that plugin will not be allowed and a compilation error will be thrown.

To support this I propose that

  1. the @kbn/optimizer writes the namespace of the public/index.{js,ts} file to the window.__kbnBundles__['plugin/${id}/public'] variable, and the namespace of the common/index.{js,ts} file to the window.__kbnBundles__['plugin/${id}/common'] variable.
    • If either of these files does not exist then imports to them will fail at runtime with a helpful error, but compilation will succeed.
  2. any plugin which depends on another plugin (via kibana.json requiredDependencies or optionalDependencies) will be able to import the code from the dependent plugin's /public or /common root.

Plugins need to declare dependencies on each other so that we can use dependency resolution order to make sure that plugin bundles are loaded into the page in the right order.

@spalger spalger added Team:Operations Team label for Operations Team enhancement New value added to drive a business result labels Apr 29, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@lukeelmers
Copy link
Member

Plugins need to declare dependencies on each other so that we can use dependency resolution order to make sure that plugin bundles are loaded into the page in the right order.

Do we have any idea yet what the rollout for this will look like, in terms of when imports start to break for plugins that have not declared explicit dependencies on items they are statically importing?

Asking because I think some plugins (data) are going to require a fair amount of planning & reorganization to make this work properly, so I want to make sure we're prepared for the change before it happens.

@spalger
Copy link
Contributor Author

spalger commented May 13, 2020

Ideally in the next month or so, I'll be tackling this in earnest once I have build metric reporting complete. All I would expect, in preparation, would be to add dependencies to kibana.json files where data wasn't listed previously, which shouldn't be happening much I would assume. What type of planning and reorganization are you expecting will be necessary?

@lukeelmers
Copy link
Member

lukeelmers commented May 14, 2020

We are expecting we will run into several circular dependencies between data and other plugins, which may require splitting some things up and shuffling static code around. So I was mostly asking to give us an idea as to how much time we have to sort this out.

We are still in the process of auditing our plugins to identify the problematic dependencies, so we should have a better idea of how challenging this will actually be in the coming weeks.

Edit: I assume this will affect both types and not just static code, correct? The types are where I think we are more likely to run into issues.

@spalger
Copy link
Contributor Author

spalger commented May 14, 2020

Yeah, I didn't expect many issues with circular dependencies after asking around a little, I should have probably asked you 😅

This will be a limitation on the actual modules webpack is injecting into the bundles, type-only imports won't be an issue (enum isn't just a type) because type-only imports are stripped from the code by Babel.

@lukeelmers
Copy link
Member

type-only imports won't be an issue

Thanks for clarifying -- if that's the case I think it'll address many of the items we were most concerned about. 👍 We may still have a few circular dependencies, but I think types were our biggest area of risk for those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Team:Operations Team label for Operations Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants