-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[new-platform] UI plugins #18874
Comments
This is how I envision it working at a high level: server-sideWhen loading Kibana, we inject a sorted array of plugin names (only UI plugins). client-sideThe core system would defer this logic to a client-side plugin service. class CoreSystem {
start() {
// ...
this.pluginService.start();
// ...
}
} The plugin service would retrieve the list of plugins from injected vars and class PluginService {
async start() {
const plugins = this.injectedMetadata.getInjectedVar('plugins');
for (const pluginName of plugins) {
const { plugin } = await loadPlugin(pluginName);
plugin().start();
}
}
} Plugin entry points would be written in the same convention as the server-side class Plugin {
start() {}
stop() {}
}
export function plugin(...args) {
return new Plugin(...args);
} The open question to me is the implementation of the For plugins that ship in Kibana, we can just use dynamic import. async function loadPlugin(name) {
return import(name);
} Since dynamic import is processed by webpack, third party plugins couldn't be A traditional async script injector using a reserved map on async function loadPlugin(name) {
return new Promise(resolve => {
const script = document.createElement('script');
script.addEventListener('load', () => {
resolve(window.__kbn__[name]);
});
script.src = name;
document.body.appendChild(script);
});
} But this would require that built plugin entry files write their exported class Plugin {
start() {}
stop() {}
}
function plugin(...args) {
return new Plugin(...args);
}
window.__kbn__.mypluginname = { plugin }; It's not clear to me what is the best way to manage this from a build tooling Thoughts? |
I agree, lets just do that
With separate webpack builds for each new platform plugin, once we get there, we'll be able to configure webpack to write the default exports of the entry file to a global on window like suggested, so we'd need to update the example to export the |
@spalger To clarify, are you proposing that we move forward with dynamic import for all plugins (core, x-pack, third party) and then only tackle the traditional script injector loader whenever we move to immutable webpack bundles for plugins? |
I'm planning on breaking this work up into 3 PRs:
|
After prototyping the Webpack-based dynamic Option 1: Magic Webpack Dynamic Bundlestl;dr: this will work, with a bit of a weird implementation hack needed to hint to Webpack what to optimize. So first of all, what actually happens when Webpack sees code like this? // src/core/public/plugins/plugin_loader.ts
import(`../../../plugins/${pluginId}/public`); Obviously, Webpack can't rely on runtime variables to know which bundles to build at compile time. So it needs to make a decision at compile time on what bundles to build. You may be thinking that we would need to configure Webpack to build these files by specifying them as entry files, but this is not how it works. In fact, when Webpack parses this file it recognizes that this This can be potentially problematic. For instance, using a very generic string like Due to this behavior, we will need to "hint" as much as we possibly can to webpack by having multiple import statements that are specific to each directory we support plugins to live in. // How we match `pluginPath` to the different paths should probably be loaded from an
// Env service based on the full directory path instead of hardcoded like this,
// but you get the idea.
let plugin;
if (pluginPath.endsWith(`kibana/src/plugins/${pluginName}`)) {
plugin = (await import(`../../../plugins/${pluginName}/public`)).plugin;
} else if (pluginPath.endsWith(`x-pack/plugins/${pluginName}`)) {
plugin = (await import(`../../../../x-pack/plugins/${pluginName}/public`)).plugin;
} else if (pluginPath.endsWith(`kibana/plugins/${pluginName}`)) {
plugin = (await import(`../../../../plugins/${pluginName}/public`)).plugin;
} else if (pluginPath.endsWith(`kibana-extra/${pluginName}`)) {
plugin = (await import(`../../../../../kibana-extra/${pluginName}/public`)).plugin;
} else {
throw new Error(`Unsupported bundle path! ${pluginPath}`);
} The other weirdness with this option is that with the Option 2: Explicit Webpack BundlesWebpack also supports not doing any magic to detect bundles that need to be built: // src/core/public/plugins/plugin_loader.ts
import(/* webpackIgnore: true */ `./plugins/${pluginId}.bundle.js`); This instead will use the normal ES2015 Loading Spec to load a bundle at a URL path relative to the bundle running the code. With this method, we would instead add the plugin paths we want to support directly to our Webpack configuration. Because these are not actual application entry points, we could make this path configuration static so that optimizer would not have to build any bundles that are ConclusionSo while the magic option would work, I prefer the explicit configuration because it gives us more explicit control, avoids any need to do preprocessing, and provides a simpler path to true immutable bundles for each plugin (in fact, if the |
More on Option 2 after some more testing: Webpack doesn't seem to support building Dynamic Modules explicitly in a way that works with older browsers unless you use their
|
I'm planning on moving forward with configuring our optimizer to create entry points that attach themselves to a global object and loading these scripts dynamically with This seems like the best approach because:
I've got a small POC of it working already and I don't anticipate the changes to optimizer to be substantial. In fact, I think this path requires less changes to the optimizer than making @epixa @spalger: Is there anything you know about that I may not be considering that I need to think about before moving forward? |
I can't think of anything off-hand. I've long imagined a similar approach to what you're describing, but I didn't know how to work it into webpack in a seamless way. |
I'm all about this if you can get this working in Kibana. I feel like I tried to load multiple entry points on the same page and ran into serious issues, but my information could be out of date or totally wrong. How does webpack handle dependencies that are shared between the multiple entry paths? |
If I'm not mistaken it uses
Webpack does its best to places shared deps in a parent chunk. AFAIR can be configured by splitChunks. |
Our splitChunks configuration in the base optimizer appears to be working fine. In my PR, I've created two different plugins that both import React and render a component and when I use |
One edge I've hit in testing, this scenario is valid:
This state causes pluginB to startup in the browser with no contract for pluginA in its dependencies. This is essentially a non-issue with plugins written in TypeScript, because you wouldn't be able to import the PluginAStart for the client-side code because it wouldn't exist. However, in a pure JS scenario, this may be unexpected and core can't throw an exception because this scenario is a totally valid use case. Options I see to solve this to make the development experience smoother:
Personally, I'm a fan of (2), but I'd like to hear others' feedback or ideas. |
Actually, I already thought about the problem and decided for myself that when you declare a dependency on a plugin you know what API you want to consume and you know it is available via contracts. Otherwise, you can also call the non-existing method and get the same problems. |
This is a good point. I think to make this more explicit we should not even populate an undefined key in the dependencies we pass to the plugin's setup function. It should just not be present at all. I'd be ok with this for now and solve for it later if it becomes more of an issue. Aside: I'm learning towards requiring that plugins at least provide a index.d.ts file if they decide they want to implement in JS.
I'm not sure why this would be an issue, could you elaborate more? |
Ok, only one case. For example,
👍 |
This was the exact scenario we had in mind when we made the original decision to make plugins an all or nothing thing rather than being able to disable or depend on only one portion of a plugin. I think the right approach here is to not include the plugin name at all in the dependencies argument as @joshdover mentioned in #18874 (comment). That's the behavior we want for optional dependencies as well, for what it's worth.
This just seems like a recipe for inaccurate types. If a plugin doesn't want to expose types, then they won't want to keep types up to date either. |
Sure if they don't want to expose types then they shouldn't return anything from their |
I don't think "not providing extension points at all" is the same thing as "not providing types for the extension point". We intend to have all of the Kibana repo be typescript, so the multiple types thing won't impact mainline development down the road. But requiring types puts an unnecessary restriction on third party development that doesn't need to exist. |
Part of #16253
See #18840 to get more details on
kibana.json
manifest and serve-side part of plugin support.details are probably going to change here
The UI plugin will look something like this:
Notice the
await import()
statement that pulls inapp.js
. This will cause webpack to split the newPlatform bundle into two chunks, one containing the bootstrapping code and one containing the app forfoo
plugin. This strategy may still be used plugin authors when they become responsible for their own build systems down the road to allow Kibana to load less code on startup.The text was updated successfully, but these errors were encountered: