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

Replace services modules by passing down references explicitly #63596

Closed
13 tasks
flash1293 opened this issue Apr 15, 2020 · 2 comments
Closed
13 tasks

Replace services modules by passing down references explicitly #63596

flash1293 opened this issue Apr 15, 2020 · 2 comments
Labels
Team:Visualizations Visualization editors, elastic-charts and infrastructure technical debt Improvement of the software architecture and operational architecture

Comments

@flash1293
Copy link
Contributor

flash1293 commented Apr 15, 2020

In a lot of places we are currently using the "services module" pattern to pass down references to global services within the plugin. In most cases the createGetterSetter utility from the utils package is used to store the references in a plugin-global module which is imported in the places the services are required.

Examples:
https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/kibana/public/discover/kibana_services.ts#L51

https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/kibana/public/visualize/kibana_services.ts#L67

https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/vis_type_table/public/services.ts#L23

This has several downsides:

  • When parts of a plugin are exported statically, they are not allowed to use this "services module", because it won't be filled with a reference as it will end up in another bundle. There is no way to statically check for these cases and it's often hard to see whether a piece of code is relying on state this way.
  • Unit tests are more difficult because the services file has to be mocked out for every test
  • It is possible to introduce lifecycle race conditions (e.g. calling a piece of code relying in the setup phase which relies on a services module populated in the start phase). These cases can't be checked for statically which means they are hard to find just by looking at the code

All these problems go away when all dependencies are passed down explicitly from the plugin class of a plugin:

  • When something is statically exported, its call signature will include the required services and every consumer will know they have to provide them. If they don't, the typescript compiler will catch the problem
  • For unit tests service mocks can simply be passed into the components to test, mocking is not necessary
  • Lifecycle race conditions will be caught by the Typescript compiler

Lens and Dashboard are doing it this way:

https://github.com/elastic/kibana/blob/master/src/plugins/dashboard/public/plugin.tsx#L247

https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/public/plugin.tsx#L90

This helper is useful in closing the gap directly in the plugin class in cases where start services have to be accessed in a sync fashion: #63720

To avoid these problems, existing code should be refactored to avoid services modules. This can happen incrementally over time.

  • input_control_vis
  • vis_type_markdown
  • vis_type_metric
  • vis_type_table
  • vis_type_tagcloud
  • vis_type_timelion
  • vis_type_timeseries
  • vis_type_vega
  • vis_type_vislib
  • discover
  • dashboard
  • visualize
  • home
@flash1293 flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Apr 15, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@stratoula stratoula added the technical debt Improvement of the software architecture and operational architecture label May 30, 2023
@stratoula
Copy link
Contributor

Thank you for contributing to this issue, however, we are closing this issue due to inactivity as part of a backlog grooming effort. If you believe this feature/bug should still be considered, please reopen with a comment.

@stratoula stratoula closed this as not planned Won't fix, can't repro, duplicate, stale Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Visualizations Visualization editors, elastic-charts and infrastructure technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

No branches or pull requests

3 participants