-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: adds support for generic and inferred typing based on graph data #7207
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
base: v5
Are you sure you want to change the base?
Conversation
export interface GraphData< | ||
N extends UnknownStruct = UnknownStruct, | ||
E extends UnknownStruct = UnknownStruct, | ||
C extends UnknownStruct = UnknownStruct, | ||
> { | ||
/** | ||
* <zh/> 节点数据 | ||
* | ||
* <en/> node data | ||
*/ | ||
nodes?: NodeData[]; | ||
nodes?: NodeData<N>[]; | ||
/** | ||
* <zh/> 边数据 | ||
* | ||
* <en/> edge data | ||
*/ | ||
edges?: EdgeData[]; | ||
edges?: EdgeData<E>[]; | ||
/** | ||
* <zh/> Combo 数据 | ||
* | ||
* <en/> combo data | ||
*/ | ||
combos?: ComboData[]; | ||
combos?: ComboData<C>[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the key starting point.
export type InferGraphDataTypes<D extends GraphData = GraphData> = { | ||
node: D['nodes'] extends NodeData<infer N>[] ? N : never; | ||
edge: D['edges'] extends EdgeData<infer E>[] ? E : never; | ||
combo: D['combos'] extends ComboData<infer C>[] ? C : never; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the key utility type that allows us to rip out the data structures from the passed in GraphData
@@ -25,7 +31,7 @@ import type { ViewportOptions } from './viewport'; | |||
* ``` | |||
*/ | |||
|
|||
export interface GraphOptions extends CanvasOptions, ViewportOptions { | |||
export interface GraphOptions<D extends GraphData = GraphData> extends CanvasOptions, ViewportOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After making the generics, we then update the core GraphOptions
type to accept the data generic
@@ -45,7 +51,7 @@ export interface GraphOptions extends CanvasOptions, ViewportOptions { | |||
* | |||
* <en/> See [Data](/en/api/data/graph-data) | |||
*/ | |||
data?: GraphData; | |||
data?: D; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The data
config value is now the generic GraphData structure D
which all other sub fields need to derive their information from.
There is a lot that needs to be done for this change since the generic GraphData needs to be inserted at all the key class definitions and interfaces. This is going to touch practically everything I suspect. But curious to get thoughts/opinions on it or if the expectation is more that the downstream consumer should just be using force casting/type coercion on their end instead? |
@thedanchez We’ve noticed your valuable contributions. The idea of adding generics to types such as GraphData and NodeData was actually considered in the initial plan for G6 5.0. However, due to the complexity this introduced to the type system, it created significant additional workload for G6 development. As a result, we had to remove those generic definitions. Currently, when working with G6 and accessing data, users need to manually assert types, which we understand can be inconvenient for TypeScript users. We acknowledge this issue and will work on optimizing the type definitions soon to improve the developer experience. |
I've carefully reviewed the code you submitted, and most of it aligns well with our ideas. However, there are some areas where adjustments could be made to reduce the scope of changes and lighten the overall workload. In my opinion, the main purpose of introducing generics is to provide better type hints for users when working with G6 — particularly for properties within the data object. I noticed that a significant portion of the implementation focuses on supporting generics for ComboOptions. This part could potentially be simplified. For instance, at this stage, we could focus solely on adding generics to ComboData, without extending it to cover ComboStyle, which would help streamline the effort. |
|
||
export class DataController { | ||
public model: GraphLib<NodeLikeData, EdgeData>; | ||
export class DataController<D extends GraphData = GraphData> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the data module is primarily used internally within G6, it should be safe to omit generics here.
|
||
export function toGraphlibData(datums: EdgeData): Edge<EdgeData>; | ||
export function toGraphlibData(datums: NodeLikeData): Node<NodeLikeData>; | ||
export function toGraphlibData<D extends GraphData = GraphData>(datums: EdgeData<InferGraphDataTypes<D>['edge']>): Edge<EdgeData<InferGraphDataTypes<D>['edge']>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toGraphlibData
is an internal utility function, so adding generics to it is not strictly necessary.
[key: string]: unknown; | ||
} | ||
|
||
type CircleComboOptions<NodeType extends UnknownStruct = UnknownStruct, ComboType extends UnknownStruct = UnknownStruct> = ComboOptionsBuilder< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that CircleComboStyleProps
and RectComboStyleProps
currently share the same properties — and are unlikely to diverge anytime soon — it might be reasonable to avoid defining separate types like CircleComboOptions
, RectComboOptions
, and CustomComboOptions
.
Summary
This PR attempts to add support for generics and full inference of graph data from the
data
object that is expected as part of constructing the graph.Adding this would elevate the developer experience as all the connected sub objects and configuration (layouts, styles, etc.) would have their data callbacks have inferred typing when reading node, edge or combo data.
Putting this up as a draft primarily as a discussion to gauge interest in this and whether there is a desire to pursue this within the repo?