[autoloading] opt into autoloading with require #5871
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
Big fan of this change! Explicit > implicit 💯 |
|
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.' |
|
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)); | |||
There was a problem hiding this comment.
Could be changed to
context.keys().forEach(context);
There was a problem hiding this comment.
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
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
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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')require('ui/autoload/directives')require('ui/autoload/filters')require('ui/autoload/modules')require('ui/autoload/all')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.