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

Clean up data plugin #39554

Closed
wants to merge 9 commits into from
Closed

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Jun 25, 2019

Summary

Working to make a clear separation between shim code & "new platform" code, so that at the end of the migration process we can just delete the shims. Also trying to make everything consistent based on the most recent conventions we have had, so that the data plugin can serve as an example for future plugins and we can point other teams to it as the recommended way of doing things.

Proposed Conventions

These changes imply a few conventions I'd like to propose (which could ultimately be added to #39542 that @joshdover started, once we get a consensus here):

  • Each plugin's server/ and public/ directory has:
    • index.ts which exports a function named plugin (already required by NP)
      • File must also export any public static code & types (already required by NP)
    • plugin.ts in a separate file which contains the plugin definition (already a recommended convention for NP).
      • This file would also export the type interfaces for Setup/Start contracts, named as NameSetup, and NameStart, and would define interfaces for it's own plugin dependencies, named as NameSetupPlugins and NameStartPlugins
      • I've updated this class to implement the proper Core-provided generic Plugin type as well
      • For clarity, class name is DataPublicPlugin (server would be DataServerPlugin). Stole this idea from @streamich 😉
    • setup.ts, which was originally added by @lizozom. This file would serve exclusively as a shim that gets imported by other plugins.
      • All shim code would live in this file, so that the plugin.ts file can remain a "clean" representation of how it will look in the new platform. In a perfect world, these shim files can simply be deleted when things move to new platform and everything Just Works(TM).
      • Other shim plugins import from here (inside their setup.ts shim files!) to get the setup contract for our shim plugin.
      • If the plugin ever introduces a start contract, a separate start.ts shim file would get created.
      • The shim file uses npSetup from ui/new_platform to inject core dependencies.

I'm working on a proposed set of conventions for static code & type exports, but the more I've experimented, the more trade-offs I run into. So for the sake of keeping this PR small (and not breaking a bunch of downstream imports), I'll branch that off into a separate PR & discussion thread.

Tasks

  • Clean up plugin definition
  • Move interpreter imports to the expressions service
  • Create separate plugin.ts file and export a plugin function from index.ts, per recommended new platform conventions
  • Remove extra static exports from plugin definition and move to index.ts
  • Pass "real" core services in via shim so we can switch over to using them when needed
  • [ ] Add shims for each individual service, since there will be legacy/shim code in those service definitions as well Not necessary yet as there aren't currently shim codes in top-level service definitions
  • Improve organization of types & other static exports <-- will move this to a separate discussion/PR

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@lukeelmers lukeelmers added review and removed WIP Work in progress labels Jul 3, 2019
@lukeelmers lukeelmers marked this pull request as ready for review July 3, 2019 22:38
@lukeelmers lukeelmers added release_note:skip Skip the PR/issue when compiling release notes v7.4.0 v8.0.0 labels Jul 3, 2019
@lukeelmers lukeelmers requested a review from a team July 3, 2019 22:39
@lukeelmers lukeelmers mentioned this pull request Jul 3, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@lizozom lizozom left a comment

Choose a reason for hiding this comment

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

In general, I love the separation between index \ plugin \ setup
I think that this should be applied immediately to the vis_markdown plugin as well, to see how it works.
And also to make sure that EPAM work with correct references.

getInterpreter: InterpreterGetter;
};
interface InterpreterGetter {
getInterpreter: () => Promise<{ interpreter: Interpreter }>;
}

/**
* Expressions Service
* @internal
*/
export class ExpressionsService {
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit unrelated but shouldn't we have registerFunction here?
I saw @ppisljar added it to expressions on markdown vis.

const coreSetup: CoreSetup = npSetup.core;

// plugin dependency shims
const pluginsSetup: DataSetupPlugins = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great. :)

@lukeelmers lukeelmers self-assigned this Jul 8, 2019
@lukeelmers
Copy link
Member Author

Going to go ahead and close this now that we've aligned on various conventions -- we can do the cleanup over the course of other refactorings that are in progress

@lukeelmers lukeelmers closed this Jul 16, 2019
@lukeelmers lukeelmers deleted the fix/data-cleanup branch February 11, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants