tisoap / react-flow-smart-edge

Custom Edge for React Flow that never intersects with other nodes
https://tisoap.github.io/react-flow-smart-edge/
MIT License
242 stars 26 forks source link

Option to propagate exceptions outside of getSmartEdge #55

Open krhsiung opened 9 months ago

krhsiung commented 9 months ago

Is your feature request related to a problem? Please describe.

We'd like to better understand the situations in which getSmartEdge returns null. We get reports through metrics of this occurring in production but have not been able to repro locally.

Describe the solution you'd like

diff --git a/src/getSmartEdge/index.ts b/src/getSmartEdge/index.ts
index 75e0f7f..3330734 100644
--- a/src/getSmartEdge/index.ts
+++ b/src/getSmartEdge/index.ts
@@ -28,6 +28,7 @@ export type GetSmartEdgeOptions = {
        nodePadding?: number
        drawEdge?: SVGDrawFunction
        generatePath?: PathFindingFunction
+       propagateError?: boolean
 }

 export type GetSmartEdgeParams<NodeDataType = unknown> = EdgeParams & {
@@ -128,7 +129,10 @@ export const getSmartEdge = <NodeDataType = unknown>({
                )

                return { svgPathString, edgeCenterX, edgeCenterY }
-       } catch {
+       } catch (e) {
+               if (options.propagateError) {
+                       throw e;
+               }
                return null
        }
 }

Describe alternatives you've considered

We could also fork the repo if this approach isn't appealing, though would love to stay on the main branch for future updates.

Additional context

Happy to open up a PR for this if I can get write permissions

tisoap commented 9 months ago

@krhsiung That's interesting, I never thought someone would want to make use of the error. However, if I were to do it, I would return a Either type or a tuple that may have the error object, as I'm not a fan of throwing exceptions. Moreover, not a fan of adding more configuration options, this library already has a lot.

I'll leave this issue open for me to investigate in the future, but it would be a breaking change if implemented.

krhsiung commented 9 months ago

Thanks for looking at this. I was able to find a repro for a graph that cannot find a smartEdge. It turns out the issue is due to the underlying pathfinding library just returning nothing - it also does does not throw.

It would be nice to have some visibility into pathfinding failure at the getSmartEdge level - it's maybe an indication that we need to tune our options, for example. However, it would mean that the smart edge library needs to package and surface its own exceptions - not as straightforward as I was hoping :(