Discussion regarding the proposed additional GraphInstantiators argument · maxGraph/maxGraph · Discussion #151
As at #150, some of the original examples of mxGraph made changes to the GraphDataModel, CellRenderer, ConnectionHandler, GraphView in-place. In this (draft) branch, I proposed that an additional argument and type be introduced to the main Graph constructor to allow having changes to behavior in these classes while not needing to make global changes which affect all future classes of that type.
export type GraphInstantiators = { GraphDataModel?: typeof GraphDataModel, GraphSelectionModel?: typeof GraphSelectionModel, Stylesheet?: typeof Stylesheet, GraphView?: typeof GraphView, CellRenderer?: typeof CellRenderer, VertexHandler?: typeof VertexHandler, EdgeHandler?: typeof EdgeHandler, EdgeSegmentHandler?: typeof EdgeSegmentHandler, ElbowEdgeHandler?: typeof ElbowEdgeHandler, }
@tbouffard raised that this approach may be inconsistent with what has been done previously:
@mcyph I don't understand the need to introduce
GraphInstantiators, at least today.There are already ways to override the
Graphdefaults:
GraphModel: pass it in the constructor- others: subclass
Graphand override thecreateXXXmethodsThis is the historical mxGraph way. We can change it but this needs more discussions IMHO.
For instance (non exhaustive list)
- with the current proposal, we now have two ways to override the default implementation: Subclass and composition. Why not keeping only one way for simplicity?
- If we remove the
createXXmethods, this will introduce more API breaks comparing to mxGraphThere are pros and cons for all solutions.
I've actually been leaning more towards create* factory functions at times, as that has been what's been used for some but not all constructor calls in mxGraph in the base Graph class, but wasn't sure whether should adopt this pattern or reuse the pattern used for the plugins argument.
I suspect the plugins argument is mainly there for tree-shaking purposes. If the create* factory functions would be preferred, I would be inclined to alter this PR to do this, as don't have strong preferences between these approaches.
I believe the fundamental problem is the way certain methods in the original Graph class instantiate hardcoded references to those classes, in particular in the middle of reasonably complex logic such as in Graph.createEdgeHandler. The original mxGraph prioritised inheritance over composition in many places, and it's made making changes like this which are not global tricky.
The options as I see them are:
- Make changes globally (previously these examples made page-global changes by altering the prototypes, so e.g. changes to EdgeSegmentHandler would make changes to all future EdgeSegmentHandler's on that page, regardless of whether they belong to the same Graph object)
- Provide alternative constructors to the subclass as attributes
- Provide more
create*factory functions in the Graph object which can be overriden - Provide the alternative constructors to the constructor of the Graph object, similar to this PR
The "alternative constructors to subclass as attributes" and "create* factory functions" options seem to both be used reasonably frequently throughout the mxGraph codebase, and the alternative constructor provided to the constructor used only occasionally.
What's everyone's thoughts on this? Are there any alternative approaches which I have missed?