feat(web_common): add components for lineage#5385
Conversation
benfdking
left a comment
benfdking
left a comment
There was a problem hiding this comment.
Wouldn't stop this but there's quite a bit that I feel like has been made quite a bit better in the vscode lineage around strictness around typing that I think this could inherit from.
|
|
||
| if (event.data.error) return errorHandler(event.data.error) | ||
|
|
||
| resolve(event.data as LayoutedGraph<TNodeData, TEdgeData>) |
There was a problem hiding this comment.
Why do we need this as here, if you have typed the input?
| const maxCount = Math.max(nodeCount, edgeCount) | ||
|
|
||
| if (nodeCount === 0) | ||
| return self.postMessage({ |
There was a problem hiding this comment.
Can we type this post message?
| for (let i = 0; i < nodeCount; i++) { | ||
| const node = nodes[i] | ||
| const width = node.width || DEFAULT_NODE_WIDTH | ||
| const height = node.height || 0 | ||
| const nodeId = node.id as NodeId | ||
| const nodeWithPosition = g.node(nodeId) | ||
| const halfWidth = width >> 1 | ||
| const halfHeight = height >> 1 | ||
|
|
||
| nodesMap[nodeId] = { | ||
| ...node, | ||
| position: { | ||
| x: nodeWithPosition.x - halfWidth, | ||
| y: nodeWithPosition.y - halfHeight, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we make this slightly more functional looking, or is it, in fact, for performance make a note of it?
| for (let i = 0; i < maxCount; i++) { | ||
| if (i < edgeCount) { | ||
| g.setEdge(edges[i].source, edges[i].target) | ||
| } | ||
| if (i < nodeCount) { | ||
| const node = nodes[i] | ||
| g.setNode(node.id, { | ||
| width: node.width || DEFAULT_NODE_WIDTH, | ||
| height: node.height || 0, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you make this into two separate loops and eliminate the concept of maxCount?
| const height = node.height || 0 | ||
| const nodeId = node.id as NodeId | ||
| const nodeWithPosition = g.node(nodeId) | ||
| const halfWidth = width >> 1 |
There was a problem hiding this comment.
I appreciate the bitwise operator for performance, but can we either note that we use this code for performance, or make a note of it
There was a problem hiding this comment.
should get rid of this
| export interface NodeBaseProps extends NodeProps { | ||
| className?: string | ||
| children?: React.ReactNode | ||
| style?: React.CSSProperties |
There was a problem hiding this comment.
I don't think you do use style?
| currentNode: LineageNode<TNodeData> | null, | ||
| selectedNodeId: NodeId | null, | ||
| selectedNodes: Set<NodeId>, | ||
| ) { |
There was a problem hiding this comment.
nit: again here I would add the output so you know what it returns.
| const nodesCount = adjacencyListKeys.length | ||
|
|
||
| if (nodesCount === 0) return {} | ||
|
|
||
| const nodesMap: LineageNodesMap<TNodeData> = Object.create(null) | ||
|
|
||
| for (let i = 0; i < nodesCount; i++) { | ||
| const nodeId = adjacencyListKeys[i] | ||
| const encodedNodeId = toNodeID(nodeId) | ||
| nodesMap[encodedNodeId] = transformNode( | ||
| encodedNodeId, | ||
| lineageDetails[nodeId], | ||
| ) | ||
| } | ||
|
|
||
| return nodesMap |
There was a problem hiding this comment.
nit: same here but make more functional to be able to or state that it is for performance
| nodeId: NodeId, | ||
| type: string = 'node', | ||
| data: TNodeData, | ||
| ) { |
There was a problem hiding this comment.
I really think we should be defining output types.
| export function toID<TReturn extends string>(...args: string[]) { | ||
| return args.join('.') as TReturn | ||
| } | ||
|
|
||
| export function toNodeID(...args: string[]) { | ||
| return encodeURI(toID(...args)) as NodeId | ||
| } | ||
|
|
||
| export function toEdgeID(...args: string[]) { | ||
| return encodeURI(toID(...args)) as EdgeId | ||
| } | ||
|
|
||
| export function toPortID(...args: string[]) { | ||
| return encodeURI(toID(...args)) as PortId | ||
| } |
There was a problem hiding this comment.
I think this isn't easy to untangle. There really shouldn't be the idea of encoded things in the lineage? Encoding is a concept we've jused to go over the network.
Can you have a look at what I have done in the vscode lineage to type these things more strictly? We should be differentiating between the different ID types and typing which ids can be mapped to what.
We should also be adding return types to functions. I wonder if we should make that a lint rule: https://typescript-eslint.io/rules/explicit-function-return-type/
mykalmax
force-pushed
the
mykalmax/web_common-lineage
branch
2 times, most recently
from
e9f204a to
d8e9a4c
Compare
mykalmax
force-pushed
the
mykalmax/web_common-lineage
branch
from
18be1ba to
27105d7
Compare
mykalmax
force-pushed
the
mykalmax/web_common-lineage
branch
from
c715cdf to
a6fbb23
Compare
2 participants
No description provided.