feat!: improve tree-shaking by isolating SelectionCellsHandler dependencies - #823
Conversation
WalkthroughThe change moves cell-handler construction from graph classes to ChangesHandler factory migration
Repository guidance updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Graph
participant SelectionCellsHandler
participant EdgeStyleRegistry
participant CellHandler
Graph->>SelectionCellsHandler: refresh selected cell states
SelectionCellsHandler->>EdgeStyleRegistry: resolve edge handler kind
SelectionCellsHandler->>CellHandler: create configured handler
SelectionCellsHandler->>SelectionCellsHandler: update or replace handler
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
12885b7 to
e1b84d6
Compare
4bcc016 to
dc4d09a
Compare
Move the cell handler factories out of AbstractGraph into the SelectionCellsHandler plugin, so that AbstractGraph no
longer references VertexHandler, EdgeHandler, ElbowEdgeHandler and EdgeSegmentHandler. Applications that do not
register the plugin, typically read-only or visualization-only ones built on BaseGraph, no longer bundle these
classes: the ts-example-without-defaults bundle drops from ~300 kB to 221 kB.
Customizing the handlers no longer requires subclassing the graph. The plugin exposes factory setters instead, which
are per instance rather than global, and which also accept the custom handler kinds registered in EdgeStyleRegistry,
something the former factory methods could not express.
Document the whole area: a new cell-handlers page on the website covering what the handlers provide, the three
global configuration objects, the mapping from handlerKind to handler and the factory setters; a handlerKind entry
in the EdgeStyleMetaData warning of the edge styles page; and consistent class comments on the four handler classes,
two of which were incomplete or pointed at the wrong configuration object.
BREAKING CHANGE:
- The cell handler factory methods no longer exist on AbstractGraph, Graph or BaseGraph: createHandler,
createEdgeHandler, createEdgeHandlerInstance, createElbowEdgeHandler, createEdgeSegmentHandler and
createVertexHandler. Retrieve the plugin with
graph.getPlugin<SelectionCellsHandler>('SelectionCellsHandler') and use its setters instead:
setVertexHandlerFactory(factory) replaces an override of createVertexHandler, and
setEdgeHandlerFactory(handlerKind, factory) replaces an override of createEdgeHandlerInstance (kind 'default'),
createElbowEdgeHandler (kind 'elbow') and createEdgeSegmentHandler (kind 'segment').
setEdgeHandlerFactoryForAllKinds(factory) sets a single factory for every kind.
- The dispatch methods createHandler and createEdgeHandler are now defined on SelectionCellsHandler. If you were
overriding them to change the dispatch logic itself, extend the plugin and pass your subclass in the plugins option.
- SelectionCellsHandler.createHandler returns a non-nullable CellHandler, whereas AbstractGraph.createHandler was
typed as nullable. TypeScript users can drop the now-useless null checks on the returned value.
dc4d09a to
b6afa3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/core/src/view/plugin/SelectionCellsHandler.ts (2)
269-276: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConfirm the
'default'entry cannot be removed.The fallback uses a non-null assertion on the
'default'factory.setEdgeHandlerFactoryandsetEdgeHandlerFactoryForAllKindsalways keep or set that key, so the assertion holds for the current public API. If a future API allows removing a kind, this assertion becomes a runtime hazard. Consider a local constant for the default factory instead of re-reading the map.
357-378: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the map deletion after the nullish check.
updateHandlerdeletes the entry before it checks whether a handler exists. The delete is a no-op for an absent key, so behavior is correct. Moving the delete after the guard makes the flow clearer and keeps the map untouched on the early-return path.♻️ Proposed reorder
updateHandler(state: CellState): void { const handler = this.handlers.get(state.cell); - this.handlers.delete(state.cell); - if (isNullish(handler)) { return; } + this.handlers.delete(state.cell);packages/core/src/view/handler/ElbowEdgeHandler.ts (1)
30-50: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReword the copied "which the built-in ... are" sentence in both handler doc blocks. Both files repeat the same sentence template to list the edge styles bound to their handler kind. The construction is hard to read and differs from the clearer "which covers ..." form already used in
EdgeHandler.ts.
packages/core/src/view/handler/ElbowEdgeHandler.ts#L30-L50: change "which the built-inelbowEdgeStyle,loopEdgeStyle,sideToSideEdgeStyleandtopToBottomEdgeStyleare" to "which covers the built-inelbowEdgeStyle,loopEdgeStyle,sideToSideEdgeStyleandtopToBottomEdgeStyle".packages/core/src/view/handler/EdgeSegmentHandler.ts#L29-L51: change "which the built-inmanhattanEdgeStyle,orthogonalEdgeStyleandsegmentEdgeStyleare" to "which covers the built-inmanhattanEdgeStyle,orthogonalEdgeStyleandsegmentEdgeStyle".packages/core/__tests__/view/plugin/SelectionCellsHandler.test.ts (2)
290-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
&&side-effect statement with anifblock.Line 295-296 uses
handlerKind != 'default' && EdgeStyleRegistry.add(...)as a statement. Lint rules such asno-unused-expressionscommonly reject this form, and anifblock states the intent directly. Also prefer!==over!=.♻️ Proposed refactor
- handlerKind != 'default' && // when not registered, it will use 'default' - EdgeStyleRegistry.add('custom', edgeStyle, { handlerKind }); + if (handlerKind !== 'default') { + // when not registered, the 'default' factory is used + EdgeStyleRegistry.add('custom', edgeStyle, { handlerKind }); + }
208-214: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the edge-style registry state for the whole file, not only this block.
unregisterAllEdgeStyles()runs inbeforeEachandafterAllfor thecreateEdgeHandlerblock. The earliercreateHandlerblock does not reset the registry, so it depends on whatever the module import left registered. Adding a top-levelbeforeEachreset removes that ordering dependency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bfc30660-3adb-4b89-859f-567b11b9904c
📒 Files selected for processing (27)
.claude/rules/architecture/coding-practices.md.claude/rules/architecture/graph-api-usage.mdCHANGELOG.mdCLAUDE.mdTODO.mdpackages/core/__tests__/view/BaseGraph.test.tspackages/core/__tests__/view/plugin/SelectionCellsHandler.test.tspackages/core/src/types.tspackages/core/src/view/AbstractGraph.tspackages/core/src/view/handler/EdgeHandler.tspackages/core/src/view/handler/EdgeSegmentHandler.tspackages/core/src/view/handler/ElbowEdgeHandler.tspackages/core/src/view/handler/VertexHandler.tspackages/core/src/view/plugin/SelectionCellsHandler.tspackages/html/stories/AutoLayout.stories.tspackages/html/stories/ContextIcons.stories.tspackages/html/stories/DragSource.stories.tspackages/html/stories/FixedPoints.stories.tspackages/html/stories/Handles.stories.tspackages/html/stories/Orthogonal.stories.tspackages/html/stories/PortRefs.stories.tspackages/html/stories/Stencils.stories.tspackages/html/stories/Wires.stories.tspackages/ts-example-without-defaults/vite.config.jspackages/website/docs/usage/cell-handlers.mdpackages/website/docs/usage/edge-styles.mdpackages/website/docs/usage/plugins.md
💤 Files with no reviewable changes (1)
- packages/core/tests/view/BaseGraph.test.ts
…s edges
The former graph.createEdgeHandler override ignored the edge style and returned CustomElbowEdgeHandler for every edge.
Migrating it to setEdgeHandlerFactory('elbow', ...) narrowed it: the two edges inserted in the model do use
'elbowEdgeStyle', but MyCustomConnectionHandler.createEdgeState builds the preview edge with 'orthogonalEdgeStyle',
and ConnectionHandler.connect copies that style onto the inserted edge. Interactively drawn edges therefore resolve to
the 'segment' handler kind and got the built-in EdgeSegmentHandler, losing MyCustomConstraintHandler (snap to fixed
points) and the isConnectableCell override (no floating connections), which are the very features this story
demonstrates.
Use setEdgeHandlerFactoryForAllKinds instead, which restores the previous behavior, and replace the misleading
"all edges use the elbow edge style" comment by one explaining why the per-kind form does not fit here.
|



Move the cell handler creation logic out of
AbstractGraphinto theSelectionCellsHandlerplugin, where it is actually used.AbstractGraphreferencedVertexHandler,EdgeHandler,ElbowEdgeHandlerandEdgeSegmentHandlerdirectly, so every application bundled them even when it never selected a cell. They are now reachable only through the plugin, so applications that do not registerSelectionCellsHandler(visualization only, for example) no longer pay for them.Configuring the cell handlers
Customizing a handler used to require subclassing the graph. The plugin now exposes factory setters, so the same customization is done by composition, on the plugin instance:
SelectionCellsHandlercreateVertexHandlersetVertexHandlerFactory(factory)createEdgeHandlerInstancesetEdgeHandlerFactory('default', factory)createElbowEdgeHandlersetEdgeHandlerFactory('elbow', factory)createEdgeSegmentHandlersetEdgeHandlerFactory('segment', factory)createEdgeHandler, when overridden to always return the same classsetEdgeHandlerFactoryForAllKinds(factory)The setters only affect handlers created after the call, so they must be called before the first selection occurs.
createHandlerandcreateEdgeHandlerremain as dispatch methods, onSelectionCellsHandlerinstead ofAbstractGraph. Overriding the dispatch logic itself is still possible, by extending the plugin and passing the subclass in thepluginsoption.Custom edge handlers by configuration only
Which
EdgeHandlergets instantiated is driven by thehandlerKindof theEdgeStyleMetaDataregistered in theEdgeStyleRegistry. Previously, only the three built-in kinds were served, and only by overriding the matchingcreate*method. A custom kind can now be handled without subclassing anything:Edge styles whose kind has no registered factory fall back to the
'default'one, so a partial configuration stays functional.Documentation
A new
usage/cell-handlers.mdpage in the website documents what the vertex and edge handlers do, the global handler configuration objects, how the handler kind is chosen, and how to configure the factories. Theedge-styles.mdandplugins.mdpages link to it.Notes
Covers #762
Impact on the size of the examples
The
nowcolumn comes from./scripts/build-all-examples.bashrun on this branch. Thebeforecolumn comes from #1126 for the TypeScript examples, which is the previous commit inmainand recalibrated them for Vite 8, and from the v0.24.0 release notes for the JavaScript examples, which are unchanged since that release as they are built with webpack and were therefore not affected by the Vite 8 upgrade.Only the
without-defaultsexamples benefit, as they are the ones that do not registerSelectionCellsHandler. The four other examples grow by 0.14 to 0.20 kB, which is the cost of the factory map and its setters now held by the plugin.ts-example-without-defaultsis the only example whosechunkSizeWarningLimitneeds an update, from 300 to 221.For reference, the first measurement in this PR, done on the 0.20.0 development version (commit 4bcc016), reported a 83.5 kB decrease, and the experiment in #449 had shown a potential decrease of 63-66 kB.
Additional measure: without subclasses of EdgeHandler
In the future (#890), we may provide a way to configure the EdgeHandler that are registered in the plugin to reduce the size of application that doesn't require them:
Remaining Tasks
DragSource: refactor: migrate the DragSource story to TypeScript #860FixedPoints: refactor: migrate the FixedPoints story to TypeScript #839PortRefs: refactor(stories): migrate PortRefs to TypeScript #1129Wires: refactor: fix most problems in the Wires story #893, also required by refactor!: move tooltip methods to TooltipHandler #640AutoLayoutContextIconsDragSourceFixedPointsHandlesOrthogonalPortRefsStencilsWiresSummary by CodeRabbit
Breaking Changes
Documentation
Tests