-
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
Update plugin generator to generate NP plugins #55281
Conversation
Pinging @elastic/kibana-operations (Team:Operations) |
Pinging @elastic/kibana-app-arch (Team:AppArch) |
What other things should be initialized with a new platform plugin? ATM it generates the following structure: plugin
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for doing this. This is looking great for now.
What other things should be initialized with a new platform plugin?
I'm not sure how far we want to go with the scaffolding, but I would probably add an endpoint in the server side and consume it from the browser side, to generate some boilerplate on how to define routes and performs http calls.
Some NITs and comments:
packages/kbn-plugin-generator/sao_template/template/kibana.json
Outdated
Show resolved
Hide resolved
packages/kbn-plugin-generator/sao_template/template/server/plugin.ts
Outdated
Show resolved
Hide resolved
packages/kbn-plugin-generator/sao_template/template/server/plugin.ts
Outdated
Show resolved
Hide resolved
packages/kbn-plugin-generator/sao_template/template/public/plugin.ts
Outdated
Show resolved
Hide resolved
packages/kbn-plugin-generator/sao_template/template/public/components/app.tsx
Outdated
Show resolved
Hide resolved
@pgayvallet I added a server side component to the generator. I also added top navigation as a non-core dependency. @elastic/kibana-design I would love some input on making the demo plugin's UI better (keeping it simple though!) |
@lizozom I've added this to our design team's meeting agenda for today. We'll discuss and get back to you! |
This is a nice onramp, easy to follow, quick to set up. In particular, I like the use of Keeping it really simple, I think we should make the following minor changes:
The end result would like something like this: Let me know if you have questions or would like to go another route. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make sure the structure here follows the "official" conventions? Specifically, the renderApp
function should live in <pluginRoot>/public/application.ts
instead of <pluginRoot>/public/components/app.ts
@streamich the scss is being used https://github.com/elastic/kibana/pull/55281/files#diff-880279824dc8e1e8b83ce12c3cc00bf0R2 The header is being added as well to internal plugins - The yarn addition is nice, but I don't want to expand the scope of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found two other small things; otherwise changes LGTM!
packages/kbn-plugin-generator/sao_template/template/public/components/app.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final comments, LGTM when addressed.
it.skip(`'yarn build' should exit 0`, async () => { | ||
await execa('yarn', ['build'], { cwd: generatedPath }); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the comment about the i18n test, what about the yarn build
and yarn lint
. If they are obsolete they should probably be removed instead of skipped?
packages/kbn-plugin-generator/sao_template/template/public/index.ts
Outdated
Show resolved
Hide resolved
packages/kbn-plugin-generator/sao_template/template/server/index.ts
Outdated
Show resolved
Hide resolved
packages/kbn-plugin-generator/sao_template/template/public/types.ts
Outdated
Show resolved
Hide resolved
// Get start services as specified in kibana.json | ||
const [coreStart, depsStart] = await core.getStartServices(); | ||
// Render the application | ||
return renderApp(coreStart, depsStart as AppPluginDependencies, params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can avoid the manual casting by specifying the plugin's dependencies:
Plugin<<%= upperCamelCaseName %>PluginSetup, <%= upperCamelCaseName %>PluginStart>
to
Plugin<<%= upperCamelCaseName %>PluginSetup, <%= upperCamelCaseName %>PluginStart>, {}, AppPluginDependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I think you also have to specify the generic on CoreSetup
on line 9, because TS does not infer types for function arguments on interface implementations.
Line 9 could be:
public setup(core: CoreSetup<AppPluginDependencies>): <%= upperCamelCaseName %>PluginSetup {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It LGTM. The question around the translations will be re-added again in a follow up PR according @lizozom
// Get start services as specified in kibana.json | ||
const [coreStart, depsStart] = await core.getStartServices(); | ||
// Render the application | ||
return renderApp(coreStart, depsStart as AppPluginDependencies, params); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I think you also have to specify the generic on CoreSetup
on line 9, because TS does not infer types for function arguments on interface implementations.
Line 9 could be:
public setup(core: CoreSetup<AppPluginDependencies>): <%= upperCamelCaseName %>PluginSetup {
@@ -1,41 +0,0 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we decide to remove this? I think it's still useful for plugins to be able to have their own dependencies and we should set them up to do that. The yarn workspaces config we have should support installing dependencies when running yarn kbn bootstrap
for plugins in this directory.
In addition, I think having them use our eslint config by default is a good idea since it helps prevent some common problems. However, they're not currently depending on it, this just relies on the plugin being in a specific directory. I suspect we won't need to require this once #53976 moves further along.
I also expect that they'll need to depend on @kbn/config-schema
in order to do anything useful on the server-side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joshdover I removed it because I was able to create and run plugins without it. I prefer to have a minimal structure, then re-introduce things based on need.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the primary audience for this package is 3rd parties. With the current system, there's a bit of complexity involved in setting up a package.json file to work with Kibana plugins (eg. the link:
dependencies). It seems to me we should help them do this, at least as an option, since I imagine most plugins will want to use some dependencies of their own (though that is an assumption).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do this in a separate follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened an issue for addressing translations and packages.json
@elasticmachine merge upstream |
@lizozom the suggestion regarding SASS was to create an |
💔 Build FailedHistory
To update your PR or re-run it, just comment with: |
* Generate NP plugin * Added tsconfig * tsconfig * Adjust sao test * Add server side to plugin gen * Added navigation * add empty element * eslint * platform team CR * design CR improvements * text updates * temp disable plugin gen tests * eslint * Code review fixes * Add scss support - requires elastic#53976 to be merged to work * CR fixes * comment fixes * Don't generate eslint for internal plugins by default * Update tests * reenable jest test for sao * Fix regex * review comments * code review Co-authored-by: Elastic Machine <[email protected]>
* Update plugin generator to generate NP plugins (#55281) * Generate NP plugin * Added tsconfig * tsconfig * Adjust sao test * Add server side to plugin gen * Added navigation * add empty element * eslint * platform team CR * design CR improvements * text updates * temp disable plugin gen tests * eslint * Code review fixes * Add scss support - requires #53976 to be merged to work * CR fixes * comment fixes * Don't generate eslint for internal plugins by default * Update tests * reenable jest test for sao * Fix regex * review comments * code review Co-authored-by: Elastic Machine <[email protected]> * Update plugin creation string Co-authored-by: Elastic Machine <[email protected]>
Summary
Following feedback from #52752
node scripts/generate_plugin.js "Banana plugin"
To generate a plugin, with the following options supported:
#57766 adds also
What's generated
The generated plugin has server side and\or client side components.
The client side features a button that, when pressed, uses the core
http
service to make a request (or if no server side generated, just shows a notification).The plugin uses the core
notifications
service to display a notification.Dev Docs
Add more options for Kibana new platform plugin generator.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers