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

New platform migration guide #31147

Merged
merged 48 commits into from
Apr 5, 2019
Merged

New platform migration guide #31147

merged 48 commits into from
Apr 5, 2019

Conversation

epixa
Copy link
Contributor

@epixa epixa commented Feb 14, 2019

@epixa epixa added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc non-issue Indicates to automation that a pull request should not appear in the release notes Feature:New Platform labels Feb 14, 2019
@epixa epixa self-assigned this Feb 14, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@epixa epixa closed this Feb 14, 2019
@epixa epixa reopened this Feb 14, 2019
src/core/MIGRATION.md Outdated Show resolved Hide resolved
@epixa epixa changed the title [skip ci] WIP New platform migration guide [skip ci] New platform migration guide Mar 16, 2019
src/core/MIGRATION.md Show resolved Hide resolved

Different service interfaces can and will be passed to `setup` and `stop` because certain functionality makes sense in the context of a running plugin while other types of functionality may have restrictions or may only make sense in the context of a plugin that is stopping.

For example, the `stop` function in the browser gets invoked as part of the `window.onbeforeunload` event, which means you can't necessarily execute asynchronous code here in a reliable way. For that reason, `core` likely wouldn't provide any asynchronous functions to plugin `stop` functions in the browser.
Copy link
Contributor

Choose a reason for hiding this comment

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

imho it makes sense to start Plugin introduction from lifecycles and describe their signature. Don't see any start mentions in the doc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, do you mean that you'd like to have a description in this document of the three lifecycle functions and how lifecycle functions work in general?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • list of lifecycles
  • their meaning
  • their signature
  • their calling order

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this would be valuable context. It would also be good to include the plugin factory function in the lifecycle calling order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed more information about lifecycles with a focus on relevant information for the sake of migration 5d343de

src/core/MIGRATION.md Show resolved Hide resolved
src/core/MIGRATION.md Outdated Show resolved Hide resolved
Bamieh and others added 2 commits March 18, 2019 17:10
- Add missing return statement highlighted during meeting.
- `apiExport` -> `uiExport`
- Small spelling check
{
"id": "demo",
"server": true,
"ui": true
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's much easier to understand and build up a mental model if the configuration option key is consistent with other naming conventions. Both should be ui or both public etc.

To dig up an old discussion, although /public is a strong convention for a folder structure, it's much less intuitive as a configuration option or as code e.g. PublicStartContract or "public": true(appears to be a visibility setting?). It's also unnatural to talk about "public code", we're more likely to say "client-side code" or "ui code".

So I'd prefer if we change public -> ui.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to leave this as-is in the guide for now since ui is what exists, but feel free to open an issue to discuss this separately.

src/core/MIGRATION.md Show resolved Hide resolved
src/core/MIGRATION.md Outdated Show resolved Hide resolved

Different service interfaces can and will be passed to `setup` and `stop` because certain functionality makes sense in the context of a running plugin while other types of functionality may have restrictions or may only make sense in the context of a plugin that is stopping.

For example, the `stop` function in the browser gets invoked as part of the `window.onbeforeunload` event, which means you can't necessarily execute asynchronous code here in a reliable way. For that reason, `core` likely wouldn't provide any asynchronous functions to plugin `stop` functions in the browser.
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this would be valuable context. It would also be good to include the plugin factory function in the lifecycle calling order.

{
"id": "demo",
"server": true,
"ui": true
Copy link
Contributor

Choose a reason for hiding this comment

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

version appears to be a required property, we might want to include "version": "8.0.0" in the example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since no plugin is quite ready to completely migrate today, I'm going to proceed as-is for now, but I'll follow up with this separately. We do still want external plugins to specify a version property, but I want to remove the requirement that new platform plugins in the Kibana repo require a version property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue for this: #34660

@epixa epixa added the review label Apr 5, 2019
@lukeelmers
Copy link
Member

I just re-read and it all makes sense to me. Only (non-urgent) suggestions I would add are:

  • maybe some more concrete examples around uiExports entry points could be added eventually. the server-side plan of action has a handful of code samples, and i think the client-side section could benefit from some code samples too (though admittedly the way this is handled on the client will vary more widely than the server).
  • in the FAQs about plugin vs package at the end, i'm wondering if it'd be worth having a brief section on "how to think about plugin boundaries," in light of some of our recent discussions around grouping plugins in terms of domains rather than treating them as highly-specialized modules

@epixa
Copy link
Contributor Author

epixa commented Apr 5, 2019

@lukeelmers Those are both great suggestions. I'm going to hold off adding them to this PR so it can go in, but they're good improvements to add to the doc in followups.

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Thanks for investing the time to do this; it's going to be a huge help for the rest of the team.

@epixa epixa merged commit a08535a into elastic:master Apr 5, 2019
@epixa epixa deleted the np-migration branch April 5, 2019 20:27
@epixa epixa changed the title [skip ci] New platform migration guide New platform migration guide Apr 5, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform non-issue Indicates to automation that a pull request should not appear in the release notes review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants