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

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

Open
3 tasks
joshuarrrr opened this issue Feb 6, 2023 · 3 comments
Open
3 tasks
Assignees
Labels
good first issue Good for newcomers help wanted Community development is encouraged technical debt If not paid, jeapardizes long-term success and maintainability of the repository. typescript

Comments

@joshuarrrr
Copy link
Member

joshuarrrr commented Feb 6, 2023

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 #3327, where the entire component rendering crashes in cases where the linkService is not yet defined.

There are also other antipatterns such as

const vegaHelpDoc = useOpenSearchDashboards().services.docLinks?.links.noDocumentation.vega;

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

Or

export const useDeps = () => useOpenSearchDashboards().services as BfetchDeps;

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

Or

const tzConfig = opensearchDashboards.services.uiSettings!.get('dateFormat:tz');

(which incorrectly uses the non-null assertion operator)

From my quick glance, it does appear to be handled correctly in other components, such as

const { services } = useOpenSearchDashboards();
if (typeof services.uiSettings !== 'object') {
throw new TypeError('uiSettings service not available in opensearch-dashboards-react context.');
}

The goals of this issue are to:

  • comprehensively survey usage of the hook
  • fix incorrect usage or type casting
  • recommend improvements to the hook itself or typing to better prevent these types of mistakes.
@joshuarrrr joshuarrrr added technical debt If not paid, jeapardizes long-term success and maintainability of the repository. typescript labels Feb 6, 2023
@joshuarrrr joshuarrrr changed the title [Tech Debt] Audit and fix usage of useOpenSearchDashboards().services.services [Tech Debt] Audit and fix usage of useOpenSearchDashboards().services Feb 6, 2023
@joshuarrrr joshuarrrr added help wanted Community development is encouraged and removed untriaged labels Feb 6, 2023
@ashwin-pc
Copy link
Member

ashwin-pc commented Mar 2, 2023

@joshuarrrr joshuarrrr added the good first issue Good for newcomers label Mar 2, 2023
@Aigerim-ai
Copy link
Contributor

Aigerim-ai commented Mar 23, 2023

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

@joshuarrrr
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Community development is encouraged technical debt If not paid, jeapardizes long-term success and maintainability of the repository. typescript
Projects
None yet
Development

No branches or pull requests

3 participants