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

[autoloading] opt into autoloading with require #5871

Merged
merged 1 commit into from
Jan 13, 2016

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Jan 10, 2016

Currently there is a long list of modules that is autoloaded for every app. This is because a number of angular directives/filters/services are not properly required by the code that needs them and rather than attempt to track down the relationships between every part of the app, autoloading makes sure that all of the defined modules are included. Since this is currently included by default it caused new apps, which didn't need anything but chrome styles, to be really heavy and slow to reoptimize.

This change makes autoloading something you opt into by requiring a ui/autoload/* module. The options include:

  • require('ui/autoload/styles')
    • include the base styles used by Kibana. This includes all of the styles listed in the ui/styles directory.
  • require('ui/autoload/directives')
  • require('ui/autoload/filters')
  • require('ui/autoload/modules')
    • include angular and several ui modules that one might expect to be loaded by default. This list is hard coded into this file.
  • require('ui/autoload/all')
    • includes all of the above lists

In order to support this change all plugins will likely need to update and include a ui/autoload/* module in their public/index.js file.

Currently there is a long list of modules that is autoloaded for every app. This is because a number of angular directives/filters/services are not properly required by the code that needs them and rather than attempt to track down the relationships between every part of the app and the rest autoloading just makes sure that all of the defined modules are included. Since this was implicit it caused new apps which didn't need anything but chrome styles to be really heavy.

This change makes autoloading something you opt into by requiring a ui/autoload/* module. The options include:
 - `ui/autoload/styles` - include the base styles used by Kibana. This includes all of the styles listed in the ui/styles directory.
 - `ui/autoload/directives`
 - `ui/autoload/filters`
 - `ui/autoload/modules` - include angular and several ui modules that one might expect to be loaded by default. This list is hard coded into this file.
 - `ui/autoload/all` - includes all of the above lists

In order to support this change all plugins will likely need to update and include a `ui/autoload/*` module in their public/index.js file.
@kimjoar
Copy link
Contributor

kimjoar commented Jan 10, 2016

Big fan of this change! Explicit > implicit

💯

@tsullivan
Copy link
Member

I agree with @kjbekkelund and looking forward to seeing this get through!

get autoload() {
console.warn(
`${this.package.id} accessed the autoload lists which are no longer available via the Plugin API.` +
'Use the `ui/autoload/*` modules instead.'
Copy link
Member

Choose a reason for hiding this comment

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

nice

@tsullivan
Copy link
Member

LGTM. Just had a question asked out of curiosity.

@@ -0,0 +1,2 @@
const context = require.context('../styles', false, /[\/\\](?!mixins|variables|_|\.)[^\/\\]+\.less/);
context.keys().forEach(key => context(key));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be changed to

context.keys().forEach(context);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I just don't like passing the other arguments (index and list). This feels concise enough.

spalger added a commit that referenced this pull request Jan 13, 2016
[autoloading] opt into autoloading with require
@spalger spalger merged commit 670c53a into elastic:master Jan 13, 2016
spalger added a commit to spalger/kibana that referenced this pull request Jan 19, 2016
With elastic#5871 we removed support for configuration-based autoloading and moved to require-based autoloading. This change is difficult for plugin authors to make in a backwards compatible way so I thought
it would make sense to start helping plugin authors know which features are/are not available to them with a simple collection of "supports" flags. For now it just shows that `autoload: false`, so that
plugins can expose an alternate main file which includes the autoloads they require
@spalger spalger deleted the implement/autoloadWithModules branch February 25, 2016 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants