opensearch-project / OpenSearch-Dashboards

📊 Open source visualization dashboards for OpenSearch.
https://opensearch.org/docs/latest/dashboards/index/
Apache License 2.0
1.65k stars 862 forks source link

[Tech Debt] Audit and fix usage of `useOpenSearchDashboards().services` #3392

Open joshuarrrr opened 1 year ago

joshuarrrr commented 1 year ago

There seems to be quite a bit of the antipattern where a component calls useOpenSearchDashboards() as a convenience hook to fetch any core service it may want to use. But the problem is that not all the services are necessarily defined. This leads to bugs like https://github.com/opensearch-project/OpenSearch-Dashboards/issues/3327, where the entire component rendering crashes in cases where the linkService is not yet defined.

There are also other antipatterns such as https://github.com/opensearch-project/OpenSearch-Dashboards/blob/5b8a378f38bdcd1223747fddfa9b791ec3071931/src/plugins/vis_type_vega/public/components/vega_help_menu.tsx#L43

(the optional chaining prevents unhandled exceptions, but it means the link href will be undefined if the service isn't found)

Or https://github.com/opensearch-project/OpenSearch-Dashboards/blob/5b8a378f38bdcd1223747fddfa9b791ec3071931/examples/bfetch_explorer/public/hooks/use_deps.ts#L34

(the type casting assures the typescript compiler that the explorer service will be defined, while the hook makes no such guarantees

Or https://github.com/opensearch-project/OpenSearch-Dashboards/blob/5b8a378f38bdcd1223747fddfa9b791ec3071931/src/plugins/data/public/ui/filter_bar/filter_editor/range_value_input.tsx#L57

(which incorrectly uses the non-null assertion operator)

From my quick glance, it does appear to be handled correctly in other components, such as https://github.com/opensearch-project/OpenSearch-Dashboards/blob/5b8a378f38bdcd1223747fddfa9b791ec3071931/src/plugins/opensearch_dashboards_react/public/ui_settings/use_ui_setting.ts#L70-L74

The goals of this issue are to:

ashwin-pc commented 1 year ago

I agree. We should migrate all such instances to use this pattern:

https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/vis_builder/public/application/components/data_tab/use/use_dropbox.tsx#L35-L41

Aigerim-ai commented 1 year ago

Hello @joshuarrrr , @ashwin-pc I would like to work on this issue, please assign it to me

joshuarrrr commented 1 year ago

OK, @Aigerim-ai, you've got it! As you investigate usage, let us know if there are other considerations or issues you discover.