feat: let register and unregister default style elements #757
Conversation
Previously, it was only possible to register the default builtins internally. "Edge Marker" factory functions, used to display start and end arrows (and other shapes), are now public. This makes it easier for applications to customize the style they need to configure. Codec unregister is also available. All new unregister functions are used within the story to reduce edge effects between stories.
WalkthroughThis pull request reorganizes the module exports and refactors the registration mechanism for codecs, styles, markers, and shapes. The changes centralize registration state management into a shared object, introduce new unregister functions for cleaning up defaults, and update import paths accordingly. Additionally, default style and marker registration in the Graph class has been revised, and licensing headers have been added to Storybook configuration files. Example projects have been updated to use the new unregistration functions. Changes
Sequence Diagram(s)sequenceDiagram
participant Graph
participant StyleRegistry
participant MarkerRegistry
Graph->>StyleRegistry: registerDefaultEdgeStyles()
StyleRegistry->>StyleRegistry: registerDefaultPerimeters()
Graph->>MarkerRegistry: registerDefaultEdgeMarkers()
sequenceDiagram
participant Config
participant CodecRegistry
participant StyleRegistry
participant ShapeRegistry
Config->>CodecRegistry: unregisterAllCodecs()
Config->>StyleRegistry: unregisterAllEdgeStylesAndPerimeters()
Config->>StyleRegistry: unregisterAllEdgeMarkers()
Config->>ShapeRegistry: unregisterAllShapes()
Suggested labels
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
packages/core/src/view/style/EdgeMarkerRegistry.ts (2)
24-65: Consider using a module with functions instead of a class with static membersThe
MarkerShapeclass only contains static members, which goes against best practices in modern JavaScript/TypeScript. Consider refactoring this to use a module with exported functions instead, which would be more aligned with functional programming principles.-class MarkerShape { - /** - * Maps from markers names to functions to paint the markers. - * - * Mapping: the attribute name on the object is the marker type, the associated value is the function to paint the marker - */ - static markers: Record<string, MarkerFactoryFunction> = {}; - - /** - * Adds a factory method that updates a given endpoint and returns a - * function to paint the marker onto the given canvas. - */ - static addMarker(type: string, funct: MarkerFactoryFunction) { - MarkerShape.markers[type] = funct; - } - - /** - * Returns a function to paint the given marker. - */ - static createMarker( - canvas: AbstractCanvas2D, - shape: Shape, - type: StyleArrowValue, - pe: Point, - unitX: number, - unitY: number, - size: number, - source: boolean, - sw: number, - filled: boolean - ) { - const markerFunction = MarkerShape.markers[type]; - return markerFunction - ? markerFunction(canvas, shape, type, pe, unitX, unitY, size, source, sw, filled) - : null; - } -} - -export default MarkerShape; +/** + * Maps from markers names to functions to paint the markers. + */ +const markers: Record<string, MarkerFactoryFunction> = {}; + +/** + * Adds a factory method that updates a given endpoint and returns a + * function to paint the marker onto the given canvas. + */ +export function addMarker(type: string, funct: MarkerFactoryFunction) { + markers[type] = funct; +} + +/** + * Returns a function to paint the given marker. + */ +export function createMarker( + canvas: AbstractCanvas2D, + shape: Shape, + type: StyleArrowValue, + pe: Point, + unitX: number, + unitY: number, + size: number, + source: boolean, + sw: number, + filled: boolean +) { + const markerFunction = markers[type]; + return markerFunction + ? markerFunction(canvas, shape, type, pe, unitX, unitY, size, source, sw, filled) + : null; +} + +export { markers };This would also require updating imports and references throughout the codebase.
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-65: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
28-28: Update comment to provide more contextThe comment mentions that the signature and name of this class will change. Consider providing more details about the planned changes or removing this comment if the changes are already implemented.
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-65: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
packages/core/src/view/style/edge-markers.ts (3)
26-27: Improve JSDoc clarity.Though these lines briefly describe the purpose of the function, consider adding a more explicit note about expected usage or example calls. This can help new contributors quickly understand how to use the marker factories.
86-88: Maintain consistent JSDoc references.These doc comments mirror the style above but ensure the wording explicitly states the primary function of
createOpenArrow(e.g., “Registers or creates open arrow shapes for edge markers”).
135-137: Clarify function description.The doc comment for
ovalcould include details about the shape’s alignment and sizing rules so others can quickly understand how to customize it, if necessary.packages/core/src/view/style/register.ts (3)
24-48: Fix possible typo in function name.
registerDefaultEdgStylesmight be missing an “e” in “Edge.” Rename this exported function to avoid confusion:-export const registerDefaultEdgStyles = (): void => { +export const registerDefaultEdgeStyles = (): void => {
71-82: Potential side effects of fully clearing StyleRegistry.
StyleRegistry.values = {}removes all custom and built-in styles. In some scenarios, users might prefer partial unregistration. Consider providing a more granular approach if there's a risk of accidental data loss.
112-121: Unregistration resets all marker definitions.
MarkerShape.markers = {}— similar to edge styles, any custom markers set after defaults were registered will also be lost. Consider a more targeted removal strategy if the library’s usage requires preserving custom markers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
packages/core/src/index.ts(3 hunks)packages/core/src/serialization/register-model-codecs.ts(3 hunks)packages/core/src/serialization/register-other-codecs.ts(5 hunks)packages/core/src/serialization/register-shared.ts(1 hunks)packages/core/src/view/Graph.ts(2 hunks)packages/core/src/view/cell/register-shapes.ts(2 hunks)packages/core/src/view/geometry/edge/ConnectorShape.ts(1 hunks)packages/core/src/view/style/EdgeMarkerRegistry.ts(1 hunks)packages/core/src/view/style/edge-markers.ts(5 hunks)packages/core/src/view/style/register.ts(2 hunks)packages/html/.storybook/main.ts(1 hunks)packages/html/.storybook/preview.ts(4 hunks)packages/js-example-without-defaults/src/index.js(2 hunks)packages/ts-example-without-defaults/src/main.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/core/src/serialization/register-model-codecs.ts (1)
packages/core/src/serialization/register-shared.ts (1)
CodecRegistrationStates(25-30)
packages/js-example-without-defaults/src/index.js (2)
packages/core/src/view/style/register.ts (2)
unregisterAllEdgeMarkers(118-121)unregisterAllEdgeStylesAndPerimeters(78-82)packages/core/src/view/cell/register-shapes.ts (1)
unregisterAllShapes(81-84)
packages/core/src/view/style/EdgeMarkerRegistry.ts (1)
packages/core/src/types.ts (2)
MarkerFactoryFunction(1258-1269)StyleArrowValue(937-937)
packages/core/src/view/Graph.ts (1)
packages/core/src/view/style/register.ts (2)
registerDefaultEdgStyles(33-48)registerDefaultPerimeters(59-69)
packages/ts-example-without-defaults/src/main.ts (2)
packages/core/src/view/style/register.ts (2)
unregisterAllEdgeMarkers(118-121)unregisterAllEdgeStylesAndPerimeters(78-82)packages/core/src/view/cell/register-shapes.ts (1)
unregisterAllShapes(81-84)
packages/core/src/view/style/register.ts (1)
packages/core/src/view/style/edge-markers.ts (4)
createArrow(29-84)createOpenArrow(90-133)oval(138-165)diamond(170-218)
🪛 Biome (1.9.4)
packages/core/src/view/style/EdgeMarkerRegistry.ts
[error] 28-65: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (windows-2022)
🔇 Additional comments (33)
packages/core/src/view/cell/register-shapes.ts (2)
39-45: Documentation update looks good.The revised documentation clearly explains what
registerDefaultShapesdoes and includes the version tag, which is helpful for tracking when this function was introduced or formalized.
73-84: Good implementation of unregister functionality.The new
unregisterAllShapesfunction provides a clean way to reset the shape registry, which aligns with the PR objective of allowing unregistration of default style elements. The implementation correctly:
- Clears the
defaultShapesobject- Resets the registration flag
This will be useful in scenarios where applications need to customize styles or reset between different contexts (as mentioned in the PR objectives).
packages/html/.storybook/main.ts (1)
1-15: License header addition is appropriate.Adding the Apache License 2.0 header ensures proper licensing attribution for this file, which is good practice for open source projects.
packages/core/src/view/geometry/edge/ConnectorShape.ts (1)
21-21: Import path change reflects appropriate refactoring.This change aligns with the PR objective of providing public access to "Edge Marker" factory functions. Moving the
MarkerShapeimport to reference the centralizedEdgeMarkerRegistrysupports better organization and accessibility of these marker functions for customization purposes.packages/core/src/serialization/register-model-codecs.ts (2)
27-27: Good centralization of codec registration states.Importing
CodecRegistrationStatesfrom a shared module improves the organization of registration state management across the codebase.
45-45: Appropriate use of shared registration state.Using the centralized
CodecRegistrationStates.modelinstead of a local variable for tracking registration status is a good refactoring that enables consistent state management. This change supports the PR objective of improving the unregistration capabilities by making registration states accessible across modules.Also applies to: 61-61
packages/core/src/serialization/register-shared.ts (3)
17-18: Clarifying the private nature of these elements is helpful.This comment accurately indicates that the elements in this file are not meant to be part of the public API, which helps prevent misuse.
22-30: Good implementation of a centralized state tracking mechanism.Using a shared object to track registration states across different codec types is a more maintainable approach than individual boolean flags. The private annotation in the JSDoc is consistent with the comment at the top of the file.
32-40: Consistent usage of the new registration state tracking.The function now properly uses the centralized state object instead of a local variable, making it more consistent with other codec registration functions.
packages/core/src/view/Graph.ts (2)
59-63: Improved organization of style registration imports.The imports have been restructured to be more specific, importing individual functions rather than a single function. This makes dependencies clearer and follows better import practices.
493-498: Better separation of concerns in style registration.The
registerDefaultsmethod now calls separate functions for edge styles and perimeters instead of a single function for all style elements. This follows the Single Responsibility Principle and makes the code more modular.packages/core/src/serialization/register-other-codecs.ts (4)
37-37: Updated import to use the new centralized registration state.The import now includes
CodecRegistrationStatesto leverage the shared registration state tracking.
72-84: Consistent usage of the centralized registration state.The function now checks and updates the core codec registration state using the shared object instead of a local variable, making it consistent with other codec registration functions.
96-105: Updated editor codecs registration to use shared state.Similar to the core codecs, the editor codecs registration now uses the centralized state management approach.
122-137: Well-implemented codec unregistration function.This new function provides a clean way to unregister all codecs and reset their registration states. The comments are clear and the implementation is straightforward, resetting both the registry data structures and the registration state flags.
packages/js-example-without-defaults/src/index.js (2)
17-25: Better API usage with dedicated unregister functions.The imports have been updated to use specific unregister functions rather than importing low-level components directly. This encapsulates implementation details and provides a cleaner API.
45-49: Improved approach to unregistering built-in defaults.Using dedicated unregister functions instead of directly manipulating internal properties is a better approach as it provides a stable API and hides implementation details. The comments also clearly explain the rationale behind unregistering these defaults.
packages/ts-example-without-defaults/src/main.ts (2)
22-24: Improved API for managing style elementsThese imports of dedicated unregister functions show a better approach to managing styles compared to the previous direct manipulation of internal objects.
45-49: API improvement for unregistering default stylesThe old approach involved direct manipulation of internal objects (
CellRenderer.defaultShapes = {}, etc.), while this new approach abstracts the unregistration logic into dedicated functions, making the code cleaner and less fragile.// Unregister maxGraph builtin style defaults // This is because Graph registers them. In the future, we expect to have an implementation of Graph that does not do it. unregisterAllEdgeMarkers(); unregisterAllEdgeStylesAndPerimeters(); unregisterAllShapes();packages/html/.storybook/preview.ts (3)
1-15: Adds standard licensing headerGood addition of the copyright and license information for the Storybook configuration files, ensuring proper attribution and licensing clarity.
35-38: Adds new unregister function importsThese imported functions provide a cleaner API for unregistering default style elements and codecs, which aligns with the PR objectives.
74-82: Improved cleanup using dedicated unregister functionsThe code now uses dedicated unregister functions instead of directly manipulating internal registry objects, making the reset functionality more maintainable and less error-prone.
packages/core/src/index.ts (3)
107-107: Updated export path for MarkerShapeThe export path for
MarkerShapehas been updated from./view/geometry/edge/MarkerShapeto./view/style/EdgeMarkerRegistry, reflecting the reorganization of code to provide better access to edge marker factory functions.
160-161: New exports for style registration and edge markersThese exports provide public access to the register functions and edge marker functionality, aligning with the PR objective to enhance customization capabilities of default style elements.
199-199: Export shape registration functionsExporting all functions from
./view/cell/register-shapesprovides users with access to shape registration functions, including the newunregisterAllShapesfunction, enhancing customization capabilities.packages/core/src/view/style/edge-markers.ts (5)
19-23: Check for consistent naming and import usage.These imports correctly reference the relevant types and constants. Ensure that all imported types (e.g.,
StyleArrowValue,AbstractCanvas2D,Shape,Point) are actually needed and used to avoid clutter.
29-31: Validate arrow function conversion.Converting from a regular function to an arrow function is appropriate for better scoping and clarity. Confirm that all references to
createArrowin the codebase are updated accordingly and ensure no calls were expecting a named function.
90-92: Check arrow function usage.Similar to
createArrow, confirm that all external references tocreateOpenArroware updated to this new exported constant form.
138-138: Exporting as a constant.Switching the shape factory to a constant arrow function improves modular clarity. Ensure there’s no performance regression due to repeated closure creation, though this is likely negligible for typical usage.
167-169: Validate diamond shape logic.The diamond marker logic correctly subtracts offset and carefully adjusts positions. Confirm that the
type === ARROW.DIAMONDdistinction for thickness is suitable for all use cases, especially if changing arrow types dynamically.Also applies to: 170-181
packages/core/src/view/style/register.ts (3)
21-22: Confirm “EdgeMarkerRegistry” import correctness.
MarkerShapeis now imported fromEdgeMarkerRegistry; verify that this is the intended module. If you plan further refactoring, consider aligning namespaces to keep them consistent across style modules.
50-69: Global state for perimeters.
isDefaultPerimetersRegisteredprevents double registration. This is fine, but confirm that callingregisterDefaultPerimeters()again in conflicting contexts (e.g., test environments with multiple runs) does not cause stale state.
84-111: Registering default markers.The approach of using the new
MarkerShape.addMarkerfor each shape is overall clear and consistent. Validate that the shape keys (e.g., “classic” vs. “block”) do not conflict with any built-in or user-defined markers.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/core/src/view/style/edge-markers.ts (1)
36-49:⚠️ Potential issueValidate
widthFactorto avoid division‑by‑zero traps
widthFactoris later used as a divisor (unitY / widthFactor,unitX / widthFactor).
If client code accidentally passes0, the generated marker factory will raise a runtimeInfinity/NaN, which is hard to trace back.export const createArrow = - (widthFactor: number) => + (widthFactor: number) => { + if (widthFactor === 0) { + throw new Error('EdgeMarker.createArrow: widthFactor must be non‑zero'); + } + return + }Apply a similar guard in
createOpenArrow.
(If0is a meaningful value, document the behaviour explicitly instead.)
🧹 Nitpick comments (7)
packages/core/src/view/style/edge-markers.ts (3)
103-116: Duplicate logic: extract a shared helper for open/filled arrows
createOpenArrowlargely duplicates the maths performed increateArrow, differing only in the path/finish logic.
Moving the common geometry into a small internal helper (e.g.computeArrowGeometry) would:
- Remove ~40 LoC duplication
- Keep the two public factories trivially readable
- Reduce the risk of the two drifting out of sync
60-66: Side‑effect onpemerits a JSDoc warningBoth factories mutate the incoming
pe(pe.x += …).
Because points are mutable references in maxGraph, callers that reuse the samePointinstance after the marker callback may observe unexpected shifts.Add a short note in the JSDoc explaining that
peis modified in‑place so users can clone beforehand if necessary.
151-178: Return‑type annotations improve DXThe factories currently return an untyped closure.
Declaring a sharedtype MarkerPainter = () => void(or re‑using an existing one if present) and annotating the return value helps IDE inference and prevents accidental misuse.packages/js-example-without-defaults/src/index.js (1)
88-97: Consistent vertex‑creation API improves readabilityThe first two vertices use
{ position: [x,y], size: [w,h] }, whereas the third uses the legacy{ x, y, width, height }shape.
Mixing both styles in one example may confuse newcomers.Either:
- switch to the modern tuple form everywhere, or
- add a comment illustrating that both syntaxes are accepted.
packages/website/docs/usage/global-configuration.md (1)
62-78: Link to API docs for faster discoveryThe new sample is great, but readers may not immediately know where the functions live.
Add inline links (or fully qualified import names) for each function, e.g.:```js import { registerDefaultEdgeMarkers, unregisterAllEdgeMarkers, } from '@maxgraph/core/view/style/register';This makes the snippet copy‑paste‑ready and reduces friction.
packages/ts-example-without-defaults/src/main.ts (1)
30-32: Consider adding more detail to the method documentationThe comment "// do nothing" is accurate but minimal. Consider expanding it to briefly explain what defaults would normally be registered by the parent class and why you're explicitly choosing not to register them in this context.
protected override registerDefaults() { - // do nothing + // do nothing - intentionally not registering default edge markers, styles, perimeters and shapes + // that would normally be registered by the Graph class }packages/html/.storybook/preview.ts (1)
61-86: Consider adding explanatory comment about the new reset approachThe function now uses a more systematic approach to reset configurations. Consider adding a comment explaining the significance of using dedicated unregister functions and how this improves maintainability.
const resetMaxGraphConfigs = (): void => { + // Using dedicated unregister functions to reset all configurations + // instead of directly manipulating registry objects. + // This ensures proper cleanup and state reset between stories. // Global configuration GlobalConfig.i18n = i18nProvider; GlobalConfig.logger = defaultLogger;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/core/src/index.ts(3 hunks)packages/core/src/view/Graph.ts(2 hunks)packages/core/src/view/cell/CellRenderer.ts(1 hunks)packages/core/src/view/cell/register-shapes.ts(2 hunks)packages/core/src/view/geometry/Shape.ts(1 hunks)packages/core/src/view/geometry/stencil/register.ts(1 hunks)packages/core/src/view/style/edge-markers.ts(5 hunks)packages/core/src/view/style/register.ts(2 hunks)packages/html/.storybook/preview.ts(4 hunks)packages/js-example-without-defaults/src/index.js(1 hunks)packages/js-example-without-defaults/src/style.css(1 hunks)packages/ts-example-without-defaults/src/main.ts(1 hunks)packages/ts-example-without-defaults/src/style.css(1 hunks)packages/website/docs/usage/codecs.md(1 hunks)packages/website/docs/usage/global-configuration.md(1 hunks)
✅ Files skipped from review due to trivial changes (6)
- packages/core/src/view/geometry/Shape.ts
- packages/js-example-without-defaults/src/style.css
- packages/website/docs/usage/codecs.md
- packages/ts-example-without-defaults/src/style.css
- packages/core/src/view/cell/CellRenderer.ts
- packages/core/src/view/geometry/stencil/register.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/core/src/view/cell/register-shapes.ts
- packages/core/src/view/style/register.ts
- packages/core/src/index.ts
- packages/core/src/view/Graph.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/js-example-without-defaults/src/index.js (3)
packages/core/src/view/Graph.ts (1)
Graph(1498-1498)packages/core/src/index.ts (1)
Graph(20-20)packages/js-example/src/index.js (4)
initializeGraph(59-73)container(80-80)graph(63-66)graph(81-81)
packages/html/.storybook/preview.ts (5)
packages/core/src/serialization/register-other-codecs.ts (1)
unregisterAllCodecs(129-137)packages/core/src/serialization/codecs/StylesheetCodec.ts (1)
StylesheetCodec(34-195)packages/core/src/view/style/register.ts (2)
unregisterAllEdgeMarkers(119-122)unregisterAllEdgeStylesAndPerimeters(79-83)packages/core/src/view/cell/register-shapes.ts (1)
unregisterAllShapes(81-84)packages/core/src/index.ts (1)
unregisterAllStencilShapes(118-118)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (windows-2022)
🔇 Additional comments (8)
packages/js-example-without-defaults/src/index.js (1)
21-33: Confirm constructor order when overridingregisterDefaultsThe trick of overriding
registerDefaultsis elegant, but relies onGraphcalling the method after the subclass v‑table is in place.
IfGraphis ever refactored to perform default registration in a static helper before the ctor finishes, the override will silently stop working.Consider adding a unit/integration test or an assertion inside the override:
registerDefaults() { // Ensure this override is actually invoked console.debug('CustomGraph.registerDefaults() skipped'); }Or expose an opt‑in flag upstream to avoid subclassing.
packages/ts-example-without-defaults/src/main.ts (2)
20-33: Great approach for preventing default style registration!Creating a custom
Graphsubclass that overridesregisterDefaultsto do nothing is a more elegant solution than the previous approach of registering defaults and then clearing them. This aligns well with the PR objective of enhancing customization capabilities.
39-43: Instantiating CustomGraph correctlyThe change to use the new CustomGraph class is properly implemented.
packages/html/.storybook/preview.ts (5)
1-15: Added appropriate license headerThe addition of the Apache 2.0 license header is correctly formatted and includes the necessary copyright information.
34-38: Good use of new unregister functionsImporting the dedicated unregister functions aligns well with the PR objectives of introducing unregister functions for default style elements.
74-76: Improved codec cleanup with centralized unregister functionUsing
unregisterAllCodecs()instead of manually clearing codec registries is more maintainable and aligns with the centralized registration state management approach.
79-83: Comprehensive cleanup of style registriesThe addition of calls to unregister edge markers, styles, perimeters, and shapes ensures a complete reset of the style-related configuration, which is important for preventing edge effects between stories.
84-86: Consistent approach for stencil shapesUsing
unregisterAllStencilShapes()instead of directly manipulating registry objects maintains consistency with the overall approach of using dedicated unregister functions.
1 participant
Previously, it was only possible to register the default builtins internally.
"Edge Marker" factory functions, used to display start and end arrows (and other shapes), are now public.
This makes it easier for applications to customize the style they need to configure.
Introduce "unregister" functions to clean the maxGraph registries:
Notes
Covers #505
Covers #760
Tasks
unregisterAllStencilShapesfunctionSummary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores