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

ComponentFactory return type is restricting #220

Open
christianvogt opened this issue Jun 11, 2024 · 1 comment
Open

ComponentFactory return type is restricting #220

christianvogt opened this issue Jun 11, 2024 · 1 comment

Comments

@christianvogt
Copy link
Collaborator

Describe the problem
Due to how ComponentFactory type is declared, components must define generic types and then cast to specific types using as. For projects where type casting is not wanted, this causes an issue.

How do you reproduce the problem?
The ComponentFactory type is declared as:

export type ComponentFactory = (
  kind: ModelKind,
  type: string
) => ComponentType<{ element: GraphElement | Graph | Edge | Node }> | undefined;

This has a side effect where components, even if they work with Edge, declare their type as element: GraphElement and then end up typecasting to their correct type as seen here: https://github.com/patternfly/react-topology/blob/main/packages/demo-app-ts/src/components/DemoDefaultEdge.tsx

I would expect to be able to define my edge component as:

type EdgeProps = {
  element: Edge;

I believe this could be solved by updating the ComponentFactory type definition:

type ComponentFactory = (
  kind: ModelKind,
  type: string,
) =>
  | ComponentType<{ element: GraphElement }>
  | ComponentType<{ element: Edge }>
  | ComponentType<{ element: Node }>
  | ComponentType<{ element: Graph }>
  | undefined;

Expected behavior
A clear and concise description of the expected behavior.

Is this issue blocking you?
Not blocking. Continue to cast.

What is your product and what release date are you targeting?
RHOAI 2.11

Any other information?

@christianvogt
Copy link
Collaborator Author

After looking into this a bit more, it seems like changing types is actually a bit problematic wrt the topology library as well as potentially breaking client component factories. The proposed solution has issues when put in practice and I haven't found any other good type mechanism.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Not started
Development

No branches or pull requests

1 participant