feat!: improve tree-shaking of EdgeStyle #809
Conversation
Remove hard-coded EdgeStyle references throughout the codebase to enable proper tree-shaking. Previously, all built-in EdgeStyle implementations were always included in application bundles. Now, only registered EdgeStyle implementations are bundled, reducing application size by up to 17kB when no EdgeStyles are used. This is implemented by: - Creating a dedicated EdgeStyleRegistry to register EdgeStyle with metadata - Introducing PerimeterRegistry to manage Perimeter registrations separately - Removing StyleRegistry which previously handled both concerns Additional improvements: - Add dedicated unregister functions for each registry type, allowing independent management of EdgeStyle and Perimeter registrations - Create BaseRegistry to share common functionality between all style registries BREAKING CHANGES: `StyleRegistry` has been removed. Use `EdgeStyleRegistry` and `PerimeterRegistry` instead. The methods of the new registries are also named differently.
WalkthroughThis change removes the generic Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EdgeStyleRegistry
participant Graph
participant EdgeHandler
participant ElbowEdgeHandler
participant EdgeSegmentHandler
User->>EdgeStyleRegistry: add('customStyle', fn, {handlerKind: 'elbow', isOrthogonal: true})
User->>Graph: createEdgeHandler(cellState)
Graph->>EdgeStyleRegistry: getHandlerKind(cellState.style.edgeStyle)
alt handlerKind == 'elbow'
Graph->>ElbowEdgeHandler: new(state)
else handlerKind == 'segment'
Graph->>EdgeSegmentHandler: new(state)
else
Graph->>EdgeHandler: new(state)
end
User->>Graph: isOrthogonal(cellState)
Graph->>EdgeStyleRegistry: isOrthogonal(cellState.style.edgeStyle)
EdgeStyleRegistry-->>Graph: true/false
Assessment against linked issues
Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
packages/core/src/view/style/marker/EdgeMarkerRegistry.ts (1)
53-58: Enhance JSDoc with explicit @param and @returns tags
The expanded description is helpful, but to improve IDE support and generated docs, please add explicit@paramentries for each method parameter and an@returnstag describing theMarkerFunction | nullreturn value.packages/core/src/view/style/edge/Orthogonal.ts (1)
147-152: Updated documentation to referenceEdgeStyleRegistry
The doc now correctly instructs users to registerOrthogonalConnectorunderorthogonalEdgeStyleinEdgeStyleRegistrywithhandlerKind: 'segment'andisOrthogonal: true.Nitpick: remove the double space before "in" on the second line.
- * **IMPORTANT**: When registering it manually in {@link EdgeStyleRegistry}, the following metadata must be used: + * **IMPORTANT**: When registering it manually in {@link EdgeStyleRegistry}, the following metadata must be used:packages/core/src/view/style/edge/Elbow.ts (1)
30-35: Documentation updated forEdgeStyleRegistryusage
The comments now referenceelbowEdgeStyleinEdgeStyleRegistryand list the required metadata (handlerKind: 'elbow',isOrthogonal: true).Nitpick: remove the extra space before "in" on the metadata line.
- * **IMPORTANT**: When registering it manually in {@link EdgeStyleRegistry}, the following metadata must be used: + * **IMPORTANT**: When registering it manually in {@link EdgeStyleRegistry}, the following metadata must be used:packages/core/src/view/style/edge/Segment.ts (1)
30-35: Documentation updated forEdgeStyleRegistryusage
The doc correctly instructs registeringSegmentConnectorundersegmentEdgeStylewithhandlerKind: 'segment'andisOrthogonal: true.Nitpick: remove the double space before "in" on the metadata line.
- * **IMPORTANT**: When registering it manually in {@link EdgeStyleRegistry}, the following metadata must be used: + * **IMPORTANT**: When registering it manually in {@link EdgeStyleRegistry}, the following metadata must be used:packages/website/docs/usage/global-configuration.md (1)
55-58: Documentation correctly updated for the new registry structure.The documentation now accurately reflects the architectural changes by:
- Listing
EdgeStyleRegistryas the dedicated registry for edge styles since 0.20.0- Listing
PerimeterRegistryas the dedicated registry for perimeters since 0.20.0- Noting that both were previously managed by
StyleRegistryThis makes the breaking change clear to users.
Fix list indentation.
The indentation of list items is inconsistent with the rest of the document.
Apply this fix:
- - `EdgeStyleRegistry`: edge styles (since 0.20.0, previously managed by `StyleRegistry`) +- `EdgeStyleRegistry`: edge styles (since 0.20.0, previously managed by `StyleRegistry`) - `CellRenderer`: shapes - `MarkerShape`: edge markers (also known as `startArrow` and `endArrow` in `CellStateStyle`) - - `PerimeterRegistry`: perimeters (since 0.20.0, previously managed by `StyleRegistry`) +- `PerimeterRegistry`: perimeters (since 0.20.0, previously managed by `StyleRegistry`) - `StencilShapeRegistry`: stencil shapes🧰 Tools
🪛 LanguageTool
[uncategorized] ~55-~55: Loose punctuation mark.
Context: ...configurations. -EdgeStyleRegistry: edge styles (since 0.20.0, previously m...(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
55-55: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
packages/core/src/view/style/edge/EdgeStyleRegistry.ts (2)
36-41: Duplicate registration silently overwrites metadata
super.add(name, edgeStyle)will replace any existing entry forname, but the previous handler/orthogonal metadata in the internal maps is not removed, leading to:
- stale metadata referenced by old
edgeStylefunctions,- inconsistent state if the same name is re-used with a different function.
Consider cleaning the old entries first:
+ const previous = this.get(name); + if (previous) { + this.handlerMapping.delete(previous); + this.orthogonalStates.delete(previous); + } super.add(name, edgeStyle);Alternatively, throw if
nameis already registered to catch mistakes early.
61-69:clear()is publicly exposed despite the JSDoc warningThe JSDoc states this should not be called directly, yet it is
publicand used by external helpers.
Either:
- Rename to
resetInternal()and make itprotected, or- Remove the warning and document that higher-level helpers may call it.
At present the API sends mixed signals to users.
packages/core/src/view/style/register.ts (2)
42-58: DefaultingisOrthogonaltotruemay hide configuration mistakesThe spread clause forces
isOrthogonaltotruewhen the caller omits the flag:isOrthogonal: metadata.isOrthogonal ?? trueWhile convenient, it silently marks all edge styles orthogonal except those explicitly set to
false(onlyloopEdgeStyletoday).
If a new non-orthogonal style is added and the author forgets to set the flag, the edge will behave differently from its visual expectation.Consider defaulting to
falseand letting the explicit opt-in convey intent:- isOrthogonal: metadata.isOrthogonal ?? true, + if (!('isOrthogonal' in metadata)) { + metadata.isOrthogonal = false; + } + EdgeStyleRegistry.add(name, edgeStyle, metadata);(or simply leave the property untouched).
101-105:unregisterAllEdgeStylesAndPerimetersname hides breaking changeThe helper now delegates to two separate unregister functions.
To avoid surprises for existing callers, consider deprecating it instead of silently changing behaviour, or update the JSDoc to highlight that it is retained only for backward compatibility.packages/core/src/types.ts (1)
1497-1521: Typo & clarity in docstringThe sentence “Defines if the edge style is considered as orthogonal or not.
@default false” misses a period and could be clearer:“Indicates whether the edge style should be treated as orthogonal.
@default false”Minor, but improves generated docs.
packages/core/__tests__/view/Graph.test.ts (1)
85-85: Remove redundant TODO commentThis TODO comment about already testing undefined values should be addressed since it creates unnecessary noise in the test file.
- ['undefined', undefined], // TODO probably already tested above + ['undefined', undefined],
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
CHANGELOG.md(1 hunks)packages/core/__tests__/util/BaseRegistry.test.ts(1 hunks)packages/core/__tests__/view/Graph.test.ts(4 hunks)packages/core/__tests__/view/GraphView.test.ts(6 hunks)packages/core/__tests__/view/style/EdgeStyleRegistry.test.ts(1 hunks)packages/core/src/index.ts(2 hunks)packages/core/src/serialization/codecs/GraphViewCodec.ts(2 hunks)packages/core/src/serialization/codecs/StylesheetCodec.ts(2 hunks)packages/core/src/serialization/codecs/utils.ts(1 hunks)packages/core/src/types.ts(5 hunks)packages/core/src/util/BaseRegistry.ts(1 hunks)packages/core/src/view/AbstractGraph.ts(7 hunks)packages/core/src/view/GraphView.ts(4 hunks)packages/core/src/view/style/StyleRegistry.ts(0 hunks)packages/core/src/view/style/edge/EdgeStyleRegistry.ts(1 hunks)packages/core/src/view/style/edge/Elbow.ts(1 hunks)packages/core/src/view/style/edge/EntityRelation.ts(1 hunks)packages/core/src/view/style/edge/Loop.ts(1 hunks)packages/core/src/view/style/edge/Manhattan.ts(1 hunks)packages/core/src/view/style/edge/Orthogonal.ts(1 hunks)packages/core/src/view/style/edge/Segment.ts(1 hunks)packages/core/src/view/style/edge/SideToSide.ts(1 hunks)packages/core/src/view/style/edge/TopToBottom.ts(1 hunks)packages/core/src/view/style/marker/EdgeMarkerRegistry.ts(1 hunks)packages/core/src/view/style/perimeter/EllipsePerimeter.ts(1 hunks)packages/core/src/view/style/perimeter/HexagonPerimeter.ts(1 hunks)packages/core/src/view/style/perimeter/PerimeterRegistry.ts(1 hunks)packages/core/src/view/style/perimeter/RectanglePerimeter.ts(1 hunks)packages/core/src/view/style/perimeter/RhombusPerimeter.ts(1 hunks)packages/core/src/view/style/perimeter/TrianglePerimeter.ts(1 hunks)packages/core/src/view/style/register.ts(4 hunks)packages/html/stories/Wires.stories.js(2 hunks)packages/js-example-selected-features/src/index.js(2 hunks)packages/ts-example-selected-features/src/main.ts(2 hunks)packages/ts-example-selected-features/vite.config.js(1 hunks)packages/ts-example-without-defaults/vite.config.js(1 hunks)packages/ts-example/vite.config.js(1 hunks)packages/website/docs/usage/edge-styles.md(4 hunks)packages/website/docs/usage/global-configuration.md(2 hunks)packages/website/docs/usage/perimeters.md(3 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/view/style/StyleRegistry.ts
🧰 Additional context used
🧠 Learnings (4)
packages/website/docs/usage/global-configuration.md (1)
Learnt from: tbouffard
PR: maxGraph/maxGraph#776
File: packages/core/src/view/Graph.ts:77-83
Timestamp: 2025-04-24T12:33:36.220Z
Learning: All register functions in maxGraph (`registerDefaultShapes`, `registerDefaultEdgeStyles`, `registerDefaultPerimeters`, and `registerDefaultEdgeMarkers`) have internal state flags to prevent duplicate registrations, making them inherently idempotent.
packages/website/docs/usage/perimeters.md (1)
Learnt from: tbouffard
PR: maxGraph/maxGraph#785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.818Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.
packages/js-example-selected-features/src/index.js (3)
Learnt from: tbouffard
PR: maxGraph/maxGraph#785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.818Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.
Learnt from: tbouffard
PR: maxGraph/maxGraph#785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.818Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.
Learnt from: tbouffard
PR: maxGraph/maxGraph#776
File: packages/core/src/view/Graph.ts:77-83
Timestamp: 2025-04-24T12:33:36.220Z
Learning: All register functions in maxGraph (`registerDefaultShapes`, `registerDefaultEdgeStyles`, `registerDefaultPerimeters`, and `registerDefaultEdgeMarkers`) have internal state flags to prevent duplicate registrations, making them inherently idempotent.
packages/core/src/view/style/perimeter/PerimeterRegistry.ts (2)
Learnt from: tbouffard
PR: maxGraph/maxGraph#785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.818Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the 'builtin-style-elements.ts' file using the statement 'export * as Perimeter from './perimeter';'. To access the Perimeter namespace in other files, they must import from './builtin-style-elements' instead of directly from './perimeter', as part of the tree-shaking optimization.
Learnt from: tbouffard
PR: maxGraph/maxGraph#785
File: packages/core/src/view/style/register.ts:19-19
Timestamp: 2025-04-28T08:24:39.818Z
Learning: In the maxGraph project, the Perimeter namespace is defined in the './builtin-style-elements' file as a namespace export ('export * as Perimeter'), making this the correct import location for accessing the Perimeter namespace.
🧬 Code Graph Analysis (13)
packages/core/src/serialization/codecs/StylesheetCodec.ts (1)
packages/core/src/serialization/codecs/utils.ts (1)
getNameFromRegistries(25-33)
packages/core/src/serialization/codecs/GraphViewCodec.ts (1)
packages/core/src/serialization/codecs/utils.ts (1)
getNameFromRegistries(25-33)
packages/ts-example-selected-features/src/main.ts (3)
packages/core/src/index.ts (1)
PerimeterRegistry(207-207)packages/core/src/view/style/perimeter/PerimeterRegistry.ts (1)
PerimeterRegistry(27-27)packages/core/src/view/style/edge/EdgeStyleRegistry.ts (1)
EdgeStyleRegistry(79-79)
packages/html/stories/Wires.stories.js (1)
packages/core/src/view/style/edge/EdgeStyleRegistry.ts (1)
EdgeStyleRegistry(79-79)
packages/core/__tests__/util/BaseRegistry.test.ts (1)
packages/core/src/util/BaseRegistry.ts (1)
BaseRegistry(23-50)
packages/core/src/serialization/codecs/utils.ts (3)
packages/core/src/view/style/edge/EdgeStyleRegistry.ts (1)
EdgeStyleRegistry(79-79)packages/core/src/index.ts (1)
PerimeterRegistry(207-207)packages/core/src/view/style/perimeter/PerimeterRegistry.ts (1)
PerimeterRegistry(27-27)
packages/js-example-selected-features/src/index.js (1)
packages/core/src/view/style/perimeter/PerimeterRegistry.ts (1)
PerimeterRegistry(27-27)
packages/core/__tests__/view/GraphView.test.ts (3)
packages/core/src/view/style/register.ts (2)
unregisterAllEdgeStyles(113-116)unregisterAllPerimeters(125-128)packages/core/src/view/style/edge/EdgeStyleRegistry.ts (1)
EdgeStyleRegistry(79-79)packages/core/src/view/style/perimeter/PerimeterRegistry.ts (1)
PerimeterRegistry(27-27)
packages/core/__tests__/view/style/EdgeStyleRegistry.test.ts (3)
packages/core/src/view/style/register.ts (1)
unregisterAllEdgeStyles(113-116)packages/core/src/types.ts (1)
EdgeStyleFunction(1276-1282)packages/core/src/view/style/edge/EdgeStyleRegistry.ts (1)
EdgeStyleRegistry(79-79)
packages/core/src/view/GraphView.ts (3)
packages/core/src/view/style/edge/EdgeStyleRegistry.ts (1)
EdgeStyleRegistry(79-79)packages/core/src/index.ts (1)
PerimeterRegistry(207-207)packages/core/src/view/style/perimeter/PerimeterRegistry.ts (1)
PerimeterRegistry(27-27)
packages/core/src/view/style/edge/EdgeStyleRegistry.ts (3)
packages/core/src/util/BaseRegistry.ts (1)
BaseRegistry(23-50)packages/core/src/types.ts (3)
EdgeStyleFunction(1276-1282)EdgeStyleHandlerKind(1497-1501)EdgeStyleMetaData(1510-1521)packages/core/src/internal/utils.ts (1)
isNullish(39-40)
packages/core/src/view/AbstractGraph.ts (2)
packages/core/src/types.ts (1)
EdgeStyleFunction(1276-1282)packages/core/src/view/style/edge/EdgeStyleRegistry.ts (1)
EdgeStyleRegistry(79-79)
packages/core/src/view/style/register.ts (3)
packages/core/src/types.ts (3)
EdgeStyleValue(1292-1300)EdgeStyleFunction(1276-1282)EdgeStyleMetaData(1510-1521)packages/core/src/view/style/edge/EdgeStyleRegistry.ts (1)
EdgeStyleRegistry(79-79)packages/core/src/view/style/perimeter/PerimeterRegistry.ts (1)
PerimeterRegistry(27-27)
🪛 LanguageTool
packages/website/docs/usage/global-configuration.md
[uncategorized] ~55-~55: Loose punctuation mark.
Context: ...configurations. - EdgeStyleRegistry: edge styles (since 0.20.0, previously m...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 markdownlint-cli2 (0.17.2)
packages/website/docs/usage/global-configuration.md
55-55: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
56-56: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
57-57: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
58-58: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (windows-2022)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (macos-14)
🔇 Additional comments (78)
packages/ts-example/vite.config.js (1)
30-30: Approve updatedchunkSizeWarningLimitvalue
The threshold has been incremented from 435 to 436 to match the current size of the@maxgraph/corechunk.packages/ts-example-without-defaults/vite.config.js (1)
30-30: Approve loweredchunkSizeWarningLimitvalue
The warning limit has been decreased from 327 to 310 in line with the observed bundle size for@maxgraph/core.packages/ts-example-selected-features/vite.config.js (1)
30-30: Approve adjustedchunkSizeWarningLimitvalue
The limit reduction from 378 to 370 aligns with the updated build output size for the core chunk.packages/core/src/view/style/perimeter/EllipsePerimeter.ts (1)
25-25: Confirm JSDoc update toPerimeterRegistry.The documentation line is correctly updated to reference
PerimeterRegistryinstead of the removedStyleRegistry, aligning with the new registry architecture.packages/core/src/view/style/perimeter/RectanglePerimeter.ts (1)
27-27: Approve JSDoc correction toPerimeterRegistry.The perimeter registration comment now accurately points to
PerimeterRegistry, matching the PR’s registry refactor.packages/core/src/view/style/perimeter/HexagonPerimeter.ts (1)
26-26: Validate JSDoc reference update.The comment for
HexagonPerimeteris now properly linked toPerimeterRegistry, consistent with the architectural change.packages/core/src/view/style/perimeter/TrianglePerimeter.ts (1)
26-26: Approve updated JSDoc toPerimeterRegistry.The triangle perimeter’s registration comment correctly references
PerimeterRegistryfollowing the removal ofStyleRegistry.packages/core/src/view/style/perimeter/RhombusPerimeter.ts (1)
26-26: Confirm JSDoc update forPerimeterRegistry.The rhombus perimeter comment is now aligned with the new
PerimeterRegistryAPI, as intended by the PR.packages/core/src/serialization/codecs/GraphViewCodec.ts (1)
18-18: Import updated for multi-registry lookup
ReplacingStyleRegistrywith the newgetNameFromRegistriesutility correctly aligns this codec with the dedicatedEdgeStyleRegistryandPerimeterRegistryapproach.CHANGELOG.md (1)
33-34: Changelog entry reflects registry refactor
The removal ofStyleRegistryand introduction ofEdgeStyleRegistry/PerimeterRegistryis clearly documented under breaking changes.packages/core/src/serialization/codecs/StylesheetCodec.ts (2)
18-18: Import updated for new registry architecture.The import has been updated to use
getNameFromRegistriesutility function instead of directly importingStyleRegistry, aligning with the new registry architecture.
88-88: Correctly migrated to use the new registry lookup utility.This change properly replaces the direct reference to
StyleRegistry.getName(value)with the newgetNameFromRegistries(value)utility function which checks multiple registries (EdgeStyleRegistry and PerimeterRegistry).This approach maintains the same functionality while enabling tree-shaking benefits described in the PR objectives.
packages/core/src/view/style/edge/EntityRelation.ts (1)
38-42: Documentation updated for new EdgeStyleRegistry structure.The documentation has been correctly updated to reference
EdgeStyleRegistryinstead ofStyleRegistryand now includes important metadata requirements for manual registration.Including the required metadata (
handlerKind: 'default'andisOrthogonal: true) is essential guidance for developers who might manually register this edge style.packages/js-example-selected-features/src/index.js (2)
30-30: Updated import to use specialized PerimeterRegistry.Correctly replaced
StyleRegistryimport with the newPerimeterRegistryimport.
42-42: Updated registration to use PerimeterRegistry.add.The registration method has been properly updated from
StyleRegistry.putValuetoPerimeterRegistry.add, maintaining the same functionality while using the new specialized registry API.This change is consistent with the PR's goal of replacing the general StyleRegistry with specialized registries for better tree-shaking.
packages/core/src/view/style/edge/Loop.ts (1)
28-32: Documentation updated for new EdgeStyleRegistry structure.The JSDoc comment has been correctly updated to reference
EdgeStyleRegistryinstead ofStyleRegistryand now includes the specific metadata requirements for manual registration.Specifying
handlerKind: 'elbow'andisOrthogonal: falseas required metadata provides valuable guidance for developers who might manually register this edge style.packages/core/src/view/style/edge/Manhattan.ts (1)
31-35: Documentation updated correctly to reflect new registry architecture.The documentation has been appropriately updated to reference the new
EdgeStyleRegistryinstead of the removedStyleRegistry, along with clear instructions about required metadata when manually registering this EdgeStyle. This change aligns with the architectural improvements for tree-shaking.packages/html/stories/Wires.stories.js (2)
70-70: Import statement correctly updated for new registry architecture.The import statement has been properly updated to use
EdgeStyleRegistryinstead of the removedStyleRegistry.
701-701: Registration method correctly updated to use new EdgeStyleRegistry.The code now properly uses
EdgeStyleRegistry.add()instead ofStyleRegistry.putValue(), which aligns with the architectural changes to improve tree-shaking.However, based on the changes in other files, you should consider adding required metadata for this EdgeStyle:
-EdgeStyleRegistry.add('wireEdgeStyle', WireConnector); +EdgeStyleRegistry.add('wireEdgeStyle', WireConnector, { + handlerKind: 'segment', // Or appropriate handler kind + isOrthogonal: true // If this is an orthogonal edge style +});packages/core/src/view/style/edge/SideToSide.ts (1)
28-32: Documentation updated correctly to reflect new registry architecture.The documentation has been appropriately updated to reference the new
EdgeStyleRegistryinstead of the removedStyleRegistry, along with clear instructions about required metadata when manually registering this EdgeStyle. This change supports the new architecture for better tree-shaking.packages/core/src/view/style/edge/TopToBottom.ts (1)
28-32: Documentation updated correctly to reflect new registry architecture.The documentation has been appropriately updated to reference the new
EdgeStyleRegistryinstead of the removedStyleRegistry, along with clear instructions about required metadata when manually registering this EdgeStyle. This change aligns with the architectural improvements for tree-shaking.packages/ts-example-selected-features/src/main.ts (2)
26-27: Import updates correctly refer to the new registriesThe imports have been updated to use the new dedicated registries (
EdgeStyleRegistryandPerimeterRegistry) instead of the now removedStyleRegistry, aligning with the architectural changes in the project.Also applies to: 33-34
49-54: Registration API properly updated with metadata supportThe perimeter and edge style registrations have been properly migrated to use the new dedicated registry APIs. The edge style registration now includes the appropriate metadata (
handlerKindandisOrthogonal), which enables improved tree-shaking as mentioned in the PR objectives.The metadata specified here is particularly important for the
orthogonalEdgeStyle, as it correctly identifies it as an orthogonal edge style and specifies the segment handler kind, which will be used by the framework to correctly process this edge style.packages/core/src/view/style/perimeter/PerimeterRegistry.ts (3)
17-19: Appropriate imports for the new registryThe imports correctly reference the
BaseRegistryclass andPerimeterFunctiontype that are needed for the implementation.
20-26: Well-documented registry with proper JSDocThe documentation clearly explains the purpose of the registry and includes appropriate JSDoc tags for categorization and versioning.
27-27: Simple and effective implementation of the registryThe implementation correctly instantiates a
BaseRegistryparametrized withPerimeterFunction, which aligns with the PR's goal of creating a dedicated registry for perimeter functions.Based on the retrieved learnings, this implementation properly supports the way Perimeter functions are managed in the codebase, helping to enable tree-shaking as mentioned in the PR objectives.
packages/website/docs/usage/perimeters.md (4)
23-25: Documentation updated for new registryThe documentation correctly mentions that perimeters are now registered in the
PerimeterRegistryinstead of the previous registry.
27-31: Clear notice about breaking changeThe added information box clearly communicates the breaking change to users, specifying that the
PerimeterRegistryis new in version 0.20.0 and replaces the previousStyleRegistry. This helps users understand and adapt to the architectural changes.
111-116: Updated string-based perimeter registration explanationThe documentation correctly explains the updated registration mechanism for string-based perimeter references, now using
PerimeterRegistryinstead of the previous registry.
151-154: Custom perimeter registration example updatedThe example for registering custom perimeters has been properly updated to use
PerimeterRegistry.add()instead of the previous method, ensuring users understand how to register custom perimeters with the new API.packages/core/__tests__/view/GraphView.test.ts (6)
22-28: Updated imports for new registries and unregister functionsThe imports have been correctly updated to include the new dedicated registries (
EdgeStyleRegistryandPerimeterRegistry) and their respective unregister functions, replacing the previous imports.
58-64: Updated edge style test cleanupThe test setup and cleanup now correctly use
unregisterAllEdgeStyles()instead of the previous method, ensuring test isolation is maintained.
88-95: Updated edge style registration in testsThe test now correctly uses
EdgeStyleRegistry.add()to register edge styles, which verifies that the new API functions properly within the application.
97-107: Edge style test with noEdgeStyle properly updatedThe test for the
noEdgeStyleproperty correctly uses the new registry API while maintaining the same functionality check.
119-126: Updated perimeter test cleanupThe perimeter test setup and cleanup now correctly use
unregisterAllPerimeters()instead of the previous method, ensuring test isolation.
140-147: Updated perimeter registration in testsThe test now correctly uses
PerimeterRegistry.add()to register perimeter functions, which verifies that the new API functions properly.packages/core/__tests__/util/BaseRegistry.test.ts (5)
1-15: License header looks good.The license header follows the standard Apache 2.0 format and properly attributes copyright to The maxGraph project Contributors.
17-18: Imports are correctly structured.The test imports the necessary Jest globals and the BaseRegistry class being tested. The import path is properly structured to access the BaseRegistry from the src directory.
20-29: Comprehensive registration test looks good.This test effectively verifies:
- Adding an object to the registry
- Retrieving a registered object by name
- Handling unknown names by returning null
- Handling edge cases (null, undefined) correctly
The test provides good coverage of the BaseRegistry's basic functionality.
31-39: Clear functionality test is well implemented.The test properly verifies that the
clearmethod removes all registry entries by:
- First adding an entry and confirming it exists
- Calling clear() method
- Verifying the entry no longer exists
This test is essential as the clear function is used by the unregister functions mentioned in the PR objectives.
41-59: getName tests are thorough and well-structured.The tests properly verify the
getNamefunctionality in two scenarios:
- When the registry is empty - confirming it returns null for any object
- When the registry contains entries - confirming it returns the correct name for a registered object
The structure using a describe block for grouping related tests follows best practices. This functionality is critical for the
getNameFromRegistriesutility that leverages this method.packages/core/src/serialization/codecs/utils.ts (3)
17-18: Correct imports for the new registry structure.The file correctly imports the new dedicated registries that replace the former StyleRegistry. This aligns with the PR objective of introducing separate registries for edge styles and perimeters.
20-20: Good approach for registry collection.Storing the registries in an array enables a clean iteration pattern in the utility function. This makes the code more maintainable if additional registries need to be added in the future.
22-33: Well-implemented utility function for cross-registry lookups.The
getNameFromRegistriesfunction:
- Is correctly versioned with
@since 0.20.0to indicate it's part of the breaking change- Efficiently iterates through each registry to find a matching name
- Returns early when a match is found for better performance
- Falls back to null if no name is found across all registries
This function is essential for code that previously relied on StyleRegistry.getName() to find style names for serialization purposes.
packages/website/docs/usage/global-configuration.md (1)
76-78: Documentation correctly updated for new unregister functions.The documentation now properly lists:
unregisterAllEdgeStyles()- new in 0.20.0unregisterAllPerimeters()- new in 0.20.0- Retains the previous
unregisterAllEdgeStylesAndPerimeters()for backwards compatibilityThis helps users understand the new, more granular approach to registry management introduced in this PR.
packages/core/src/view/GraphView.ts (4)
45-46: Imports correctly updated to use the new registry structure.The imports properly reference the new dedicated registries:
EdgeStyleRegistryfor edge stylesPerimeterRegistryfor perimetersThis is a key part of implementing the registry split mentioned in the PR objectives.
138-140: Documentation comment correctly updated for allowEval property.The comment now accurately reflects that the new
EdgeStyleRegistryandPerimeterRegistryare used when resolving style values, replacing the previous reference to StyleRegistry. This ensures the documentation stays in sync with the implementation changes.
1327-1336: Edge style resolution updated to use EdgeStyleRegistry.The code correctly:
- Replaces the previous StyleRegistry lookup with EdgeStyleRegistry.get()
- Maintains the fallback to eval when necessary
- Preserves the same behavior while using the new registry structure
This change implements a key part of the PR objective to improve tree-shaking by removing hard-coded EdgeStyle references.
1596-1602: Perimeter function resolution updated to use PerimeterRegistry.The code correctly:
- Replaces the previous StyleRegistry lookup with PerimeterRegistry.get()
- Maintains the fallback to eval when necessary
- Preserves the same behavior while using the new registry structure
This change completes the implementation of the registry split mentioned in the PR objectives.
packages/core/__tests__/view/style/EdgeStyleRegistry.test.ts (7)
17-24: Good use of imports and test setupThe imports correctly include all the necessary components from the core library, including
EdgeStyle,EdgeStyleFunction,EdgeStyleRegistry, andunregisterAllEdgeStyles. This sets up the tests for the new registry properly.
25-33: Excellent test isolation practiceThe warning comment and use of
beforeEachandafterAllhooks to clear the registry state prevents side effects between tests and other test suites. This is critical when testing components that rely on global state.
35-38: Good test helper setupThe custom edge style function is properly defined with the correct type signature (
EdgeStyleFunction), and the empty implementation is sufficient for testing purposes since we're only testing registration functionality.
39-48: Complete metadata registration testThis test properly verifies that edge styles can be registered with metadata and that all metadata properties (
isOrthogonalandhandlerKind) are correctly retrieved. The use of direct comparisons and boolean assertions is appropriate.
50-56: Good default values testThis test verifies that registering an edge style without metadata results in correct default values (
handlerKindis 'default' andisOrthogonalis false). This ensures the registry behaves predictably when metadata is omitted.
58-71: Thorough clear functionality testThe test properly verifies that the
clearmethod removes all registered styles and resets their metadata to default values. This is important for ensuring that the registry can be properly reset between tests or during application lifecycle.
73-88: Comprehensive getName testsThe tests for the
getNamemethod cover both the case where no elements are registered and where multiple elements are registered. The tests properly verify that the correct name is returned for a registered function and thatnullis returned for unregistered functions or null input.packages/core/src/index.ts (3)
184-184: Properly exposing BaseRegistry in public APIGood addition of the BaseRegistry export which makes the generic registry functionality available to consumers.
205-205: Correctly exposing EdgeStyleRegistryThe export for edge style registry functionality is properly added, making the new registry available to consumers.
207-207: Properly exposing PerimeterRegistryThe named export for PerimeterRegistry correctly replaces the previous StyleRegistry, completing the transition to dedicated registries.
packages/website/docs/usage/edge-styles.md (6)
21-21: Updated documentation to reflect new registryThe documentation correctly reflects that EdgeStyles are now registered in the
EdgeStyleRegistryinstead of the previousStyleRegistry.
25-29: Clear version change informationGood addition of an info box explaining the introduction of
EdgeStyleRegistryin version 0.20.0 and the removal ofStyleRegistry. This helps users understand the breaking changes.
39-39: Updated example to reference EdgeStyleRegistryThe example now correctly references the new
EdgeStyleRegistryfor the built-in EdgeStyle registration.
51-51: Consistent registry namingThe documentation for configuring default edge styles has been updated to reference the new
EdgeStyleRegistry, maintaining consistency throughout the document.
83-90: Clear metadata registration exampleThe example for registering a custom edge style now includes the important metadata parameters (
handlerKindandisOrthogonal). This is crucial for ensuring that custom edge styles work correctly with the framework.
92-95: Important warning about metadata registrationExcellent addition of a warning about the importance of registering EdgeStyles with correct metadata. This will help prevent subtle bugs when users create custom edge styles.
packages/core/src/util/BaseRegistry.ts (5)
17-22: Well-documented class with proper JSDoc annotationsThe class includes comprehensive JSDoc comments with appropriate category tags and version information, making it clear that this is a new addition in version 0.20.0.
23-25: Good use of generics for type safetyThe
BaseRegistry<V>class properly uses TypeScript generics to provide type safety for the values stored in the registry. The protected map is a good implementation choice for the storage.
26-33: Simple and effective add/get methodsThe add and get methods are straightforward and handle null/undefined inputs gracefully. The null fallback in the get method is a good defensive programming practice.
34-41: Efficient getName implementationThe
getNamemethod correctly handles the lookup of a style's name by iterating through entries. The early return when a match is found is efficient, and returning null for unregistered values is consistent with the get method.
43-49: Clear warning about clear method usageThe clear method is properly documented with a warning that it should not be called directly, encouraging users to use the global unregister functions instead. This helps maintain the integrity of the registry state.
packages/core/__tests__/view/Graph.test.ts (8)
17-34: Good addition of registry-related importsThe additional imports for
registerDefaultEdgeStylesandunregisterAllEdgeStylesalign well with the PR's objective of introducing dedicated registries. These functions are essential for the test setup to properly isolate and test behavior with and without registered edge styles.
58-91: Well-structured test isolation for registered stylesThe addition of this nested describe block with proper setup and teardown is excellent. This clearly separates tests for when default edge styles are registered, ensuring clean test isolation. The beforeEach/afterAll pattern ensures each test runs with a clean state.
93-112: Clear test organization for unregistered stylesGood refactoring of these tests to explicitly indicate they're for the case when default built-in styles are NOT registered. This complements the earlier describe block and makes the test scenarios much clearer.
115-126: Good extraction of helper functionsThe refactoring of
createCellStateto an arrow function and addition of thecreateCellStateOfEdgehelper function improves code readability and reduces duplication in the tests. This is a good pattern for test utilities.
129-172: Comprehensive testing of handler creationThe new test cases effectively verify that the
createEdgeHandlermethod properly delegates to the registry to determine which handler type to use based on the registered style metadata. The tests are well-structured and cover all expected handler types for different edge styles.
186-187: Clear test naming for unregistered styles scenarioGood modification of the test description to explicitly indicate it's testing the scenario when default built-in styles are NOT registered. This naming helps clearly differentiate between the two test scenarios.
190-190: Good use of utility function for precise assertionUsing the
expectExactInstanceOfEdgeHandlerutility function here provides a more precise assertion than just checking forinstanceof EdgeHandler, as it explicitly verifies the handler is not a subclass.
209-213: Excellent utility function for precise type assertionsThe addition of
expectExactInstanceOfEdgeHandleris a great improvement. It not only checks that the handler is an instance ofEdgeHandler, but also verifies it's not a subclass, which is crucial for these tests. This kind of utility function enhances test precision and readability.
No longer tries to do it with object in GraphViewCodec. The related registries only hold function. In addition, the logic was wrong. It should have been a OR instead of a AND when checking for 'function' and 'object'.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/core/src/internal/BaseRegistry.ts (2)
31-33: Consider refactoring to avoid non-null assertionThe
getmethod uses a non-null assertion operator (!) onnamewhich could hide potential issues. While the nullish coalescing operator handles the case afterward, it would be clearer to check ifnameis a valid string before trying to get it from the map.get(name: string | null | undefined): V | null { - return this.values.get(name!) ?? null; + if (name === null || name === undefined) { + return null; + } + return this.values.get(name) ?? null; }
35-42: Consider performance implications for large registriesThe
getNamemethod performs a linear search through all entries. This implementation is straightforward and works well for small registries, but could become a performance bottleneck if the registry grows large.For the current use case of style registries, this is likely acceptable since the number of registered styles is typically small. If performance becomes an issue, consider maintaining a reverse lookup map.
packages/core/__tests__/internal/BaseRegistry.test.ts (3)
2-2: Update the copyright year to 2024The copyright year is set to 2025, which is in the future. This should be updated to the current year (2024) or a range (e.g., "2023-2024") if the project started earlier.
-Copyright 2025-present The maxGraph project Contributors +Copyright 2024-present The maxGraph project Contributors
21-21: Consider adding type parameter for better type safetyThe
BaseRegistryis instantiated without specifying the generic type parameter. While TypeScript can infer the type in this case, explicitly specifying it would improve code clarity and type safety.- const baseRegistry = new BaseRegistry(); + const baseRegistry = new BaseRegistry<Record<string, string>>();
1-60: Consider adding tests for registry entry overridesThe current tests don't verify what happens when you register multiple values with the same key. According to the
BaseRegistryimplementation, the latest registration should override previous ones, but this behavior isn't tested.test('registration override', () => { const baseRegistry = new BaseRegistry<Record<string, string>>(); const object1 = { property: 'value1' }; const object2 = { property: 'value2' }; baseRegistry.add('name', object1); expect(baseRegistry.get('name')).toBe(object1); baseRegistry.add('name', object2); expect(baseRegistry.get('name')).toBe(object2); expect(baseRegistry.get('name')).not.toBe(object1); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/core/__tests__/internal/BaseRegistry.test.ts(1 hunks)packages/core/src/index.ts(1 hunks)packages/core/src/internal/BaseRegistry.ts(1 hunks)packages/core/src/types.ts(5 hunks)packages/core/src/view/style/edge/EdgeStyleRegistry.ts(1 hunks)packages/core/src/view/style/perimeter/PerimeterRegistry.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/core/src/view/style/edge/EdgeStyleRegistry.ts
- packages/core/src/view/style/perimeter/PerimeterRegistry.ts
- packages/core/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/src/types.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/core/__tests__/internal/BaseRegistry.test.ts (1)
packages/core/src/internal/BaseRegistry.ts (1)
BaseRegistry(24-47)
packages/core/src/internal/BaseRegistry.ts (1)
packages/core/src/types.ts (1)
Registry(1496-1508)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build
- GitHub Check: build (windows-2022)
🔇 Additional comments (4)
packages/core/src/internal/BaseRegistry.ts (1)
24-47: Good implementation of base registryThis foundational
BaseRegistryclass provides a simple yet effective implementation of theRegistryinterface. The protectedvaluesmap enables subclasses to extend functionality while maintaining encapsulation.The class provides all the necessary operations (add, get, getName, clear) with appropriate null handling. The
@privateand version annotations properly document that this is an internal implementation detail.packages/core/__tests__/internal/BaseRegistry.test.ts (3)
20-29: LGTM! Good test coverage for basic registry functionality.This test thoroughly verifies the core registration functionality, including edge cases for non-existent keys, null, and undefined inputs. The test ensures the registry correctly returns the registered object for valid keys and null for invalid ones.
31-39: LGTM! Clear test verifies registry reset functionality.This test properly verifies that the
clearmethod removes all registered entries from the registry.
41-59: LGTM! Comprehensive tests for getName functionality.These tests thoroughly verify the
getNamemethod in both scenarios - when the registry is empty and when it contains multiple entries. Good job testing the null input case in both scenarios.
|
Remove hard-coded EdgeStyle references throughout the codebase to enable proper tree-shaking.
Previously, all built-in EdgeStyle implementations were always included in application bundles.
Now, only registered EdgeStyle implementations are bundled, reducing application size by up to 17kB when no EdgeStyles are used.
This is implemented by:
Additional improvements:
BREAKING CHANGES:
StyleRegistryhas been removed. UseEdgeStyleRegistryandPerimeterRegistryinstead.The methods of the new registries are also named differently.
Notes
Closes #767
Impact on the size of the examples
Analysis
EdgeStyle.OrthConnectorwhich depends onEdgeStyle.Segment(which is then bundled). The decrease is about 8 kB.Summary by CodeRabbit
Summary by CodeRabbit
New Features
EdgeStyleRegistryandPerimeterRegistry.Bug Fixes
Documentation
EdgeStyleRegistryandPerimeterRegistryinstead of the removedStyleRegistry.Chores