refactor!: remove extra getDefault functions and related functions #662
Conversation
BREAKING CHANGES: Some utility functions formerly used to retrieve default value of `CellStateStyle` and `CellStyle` properties, mainly intended to only be used internally, have been removed. Use the nullish coalescing operator (??) and the optional chaining (?.) instead. Removed functions: - `utils.getValue` - `stringUtils.getColor` - `stringUtils.getNumber` - `stringUtils.getStringValue`
|
Warning Rate limit exceeded@tbouffard has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThe changes remove several legacy utility functions used for default value retrieval and update various modules to utilize optional chaining and nullish coalescing. A new test suite for cell foldability has been added, and several documentation comments and type annotations have been updated. Build configuration thresholds have been slightly adjusted. Additionally, the migration guide has been revised to reflect breaking changes such as renamed classes, methods, and API signatures. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as Test Suite
participant FoldingMixin
participant Cell
TestSuite->>FoldingMixin: call isCellFoldable(Cell, _collapse?)
FoldingMixin->>Cell: getCurrentCellStyle()
Cell-->>FoldingMixin: return cell style (including foldable flag)
FoldingMixin-->>TestSuite: return foldability result
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/core/__tests__/view/mixins/FoldingMixin.test.ts (2)
21-21: Consider renaming the parameter for clarity.The constant
ignoredFoldableParametercould be renamed to better reflect its purpose in the test cases.-const ignoredFoldableParameter = false; +const collapseFoldableParameter = false; // Parameter for isCellFoldable's collapse argument
29-60: Test suite provides good coverage but could be enhanced.The test suite covers essential scenarios but could benefit from additional edge cases.
Consider adding these test cases:
test('Using Cell with undefined style', () => { expect( createGraphWithoutPlugins().isCellFoldable( createCellWithChildren(undefined), ignoredFoldableParameter ) ).toBeTruthy(); }); test('Using Cell with null style', () => { expect( createGraphWithoutPlugins().isCellFoldable( createCellWithChildren(null), ignoredFoldableParameter ) ).toBeTruthy(); });packages/html/stories/FixedIcons.stories.js (1)
65-65: Consider using a constant for the shadow color.The hardcoded color value could be moved to a constants file for better maintainability.
-StyleDefaultsConfig.shadowColor = '#C0C0C0'; +StyleDefaultsConfig.shadowColor = STYLE_CONSTANTS.SHADOW_COLOR;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
CHANGELOG.md(1 hunks)packages/core/__tests__/view/mixins/FoldingMixin.test.ts(1 hunks)packages/core/src/types.ts(1 hunks)packages/core/src/util/StringUtils.ts(0 hunks)packages/core/src/util/Utils.ts(1 hunks)packages/core/src/util/mathUtils.ts(5 hunks)packages/core/src/view/cell/Cell.ts(1 hunks)packages/core/src/view/geometry/edge/ArrowConnectorShape.ts(1 hunks)packages/core/src/view/geometry/node/StencilShape.ts(3 hunks)packages/core/src/view/handler/CellEditorHandler.ts(4 hunks)packages/core/src/view/layout/StackLayout.ts(2 hunks)packages/core/src/view/mixins/FoldingMixin.ts(3 hunks)packages/core/src/view/style/EdgeStyle.ts(1 hunks)packages/core/src/view/style/edge/Loop.ts(1 hunks)packages/html/stories/FixedIcons.stories.js(1 hunks)packages/html/stories/Handles.stories.ts(6 hunks)packages/ts-example-without-defaults/vite.config.js(1 hunks)packages/ts-example/vite.config.js(1 hunks)packages/website/docs/usage/migrate-from-mxgraph.md(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/src/util/StringUtils.ts
✅ Files skipped from review due to trivial changes (3)
- packages/ts-example/vite.config.js
- packages/ts-example-without-defaults/vite.config.js
- packages/core/src/view/cell/Cell.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[formatting] ~15-~15: Did you mean “?”
Context: ...cing) and the [Optional chaining (?.)](https://developer.mozilla.org/en-US/d...
(MULTIPLE_PUNCTATION_MARKS_1)
packages/website/docs/usage/migrate-from-mxgraph.md
[formatting] ~196-~196: Did you mean “?”
Context: ...coalescing) and the [Optional chaining (?.)](https://developer.mozilla.org/en-US/d...
(MULTIPLE_PUNCTATION_MARKS_1)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
14-14: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (windows-2022)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (24)
packages/core/__tests__/view/mixins/FoldingMixin.test.ts (1)
23-27: LGTM! Well-structured helper function.The helper function is well-implemented with proper type annotations and default value handling.
packages/html/stories/FixedIcons.stories.js (1)
54-55: LGTM! Good use of nullish coalescing operator.The replacement of
utils.getValuewith the nullish coalescing operator improves code readability and aligns with modern JavaScript practices.packages/core/src/view/style/edge/Loop.ts (1)
67-68: LGTM! Good use of nullish coalescing and optional chaining.The replacement of
utils.getValuewith nullish coalescing and optional chaining operators improves code readability. The optional chaining ondirectionis particularly appropriate as it safely handles potential undefined style objects.packages/core/src/util/Utils.ts (1)
57-60: LGTM! Type expansion improves function versatility.Adding
booleanto the union type forisNullishandisNotNullishfunctions enhances their utility while maintaining type safety.Let's verify if there are any existing usages of these functions with boolean values:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for usages of isNullish/isNotNullish with boolean values rg "is(Not)?Nullish\((true|false)" -A 2Length of output: 41
No explicit boolean literal invocations found for isNullish/isNotNullish
- The current search for literal boolean values (i.e.
trueorfalse) in calls to these functions returned no matches.- This indicates that, while the union type now accepts booleans, there are no existing usages in the codebase that explicitly pass boolean literals.
- The change maintains type safety and expands versatility without introducing any conflicts.
packages/core/src/view/mixins/FoldingMixin.ts (2)
81-82: LGTM! Clean and concise implementation.The changes improve code clarity by:
- Indicating unused parameter with
_prefix- Using nullish coalescing for cleaner default value handling
169-169: LGTM! Improved property access pattern.The changes enhance code readability by:
- Marking unused parameter with
_prefix- Using nullish coalescing for default value handling
Also applies to: 182-182
packages/html/stories/Handles.stories.ts (2)
72-74: LGTM! Enhanced type safety and consistent default handling.The changes improve code quality by:
- Adding proper type assertions for better type safety
- Using nullish coalescing with prototype defaults consistently
Also applies to: 91-93
132-133: LGTM! Consistent implementation of property access.The changes maintain a consistent pattern of:
- Type assertion for style object
- Nullish coalescing for default values
Also applies to: 139-139, 148-149, 170-171, 186-187
packages/core/src/view/geometry/edge/ArrowConnectorShape.ts (1)
89-90: LGTM! Removed redundant type assertions.The changes improve code cleanliness by removing unnecessary type assertions while maintaining type safety through TypeScript's type inference.
packages/core/src/view/layout/StackLayout.ts (1)
299-300: LGTM! Consistent use of nullish coalescing.The changes improve code readability by:
- Using nullish coalescing for default values consistently
- Removing dependency on utility functions
Also applies to: 360-360
packages/core/src/view/geometry/node/StencilShape.ts (3)
23-23: LGTM! Import changes align with PR objectives.The removal of utility function imports and addition of
isNotNullishaligns with the PR's goal to remove extra utility functions.
223-224: LGTM! Property access updated to use optional chaining.The changes correctly use optional chaining and nullish coalescing for accessing style properties:
directionfromshape.style?.directionstrokeWidthfromshape.style?.strokeWidth ?? 1Also applies to: 228-229
529-530: LGTM! Property access updated to use optional chaining.The changes correctly use optional chaining and nullish coalescing for accessing style properties:
flipHfromshape.style?.flipH ?? falseflipVfromshape.style?.flipV ?? falsepackages/core/src/view/handler/CellEditorHandler.ts (3)
41-41: LGTM! Import changes align with PR objectives.The removal of utility function imports and keeping only the required functions aligns with the PR's goal.
533-534: LGTM! Property access updated to use optional chaining.The changes correctly use optional chaining and nullish coalescing for accessing style properties:
const lw = state.style.labelWidth ?? null; this.align ?? state.style.align ?? ALIGN.CENTER, state.style.verticalAlign ?? ALIGN.MIDDLEAlso applies to: 538-539
558-559: LGTM! Property access updated to use optional chaining.The changes correctly use optional chaining and nullish coalescing for accessing style properties:
let hpos = state.style.labelPosition ?? ALIGN.CENTER; let vpos = state.style.verticalLabelPosition ?? ALIGN.MIDDLE; const horizontal = state.style.labelPosition ?? ALIGN.CENTER;Also applies to: 1005-1005
packages/core/src/types.ts (1)
172-174: LGTM! Documentation improvement for direction property.The updated documentation clarifies that the
directionstyle is used for both shapes (e.g., Swimlane) and edge styles (e.g., Loop), with their respective default values.packages/core/src/view/style/EdgeStyle.ts (1)
251-273: LGTM! Clean refactor using nullish coalescing.The changes effectively replace utility function calls with modern JavaScript syntax using the nullish coalescing operator (??), making the code more concise and maintainable while preserving the same functionality.
CHANGELOG.md (1)
12-20: LGTM! Clear documentation of breaking changes.The changelog entry effectively communicates:
- The breaking changes being introduced
- The rationale behind the changes
- Clear migration guidance for users
🧰 Tools
🪛 LanguageTool
[formatting] ~15-~15: Did you mean “?”
Context: ...cing) and the [Optional chaining (?.)](https://developer.mozilla.org/en-US/d...(MULTIPLE_PUNCTATION_MARKS_1)
🪛 markdownlint-cli2 (0.17.2)
14-14: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 4; Actual: 8(MD007, ul-indent)
packages/website/docs/usage/migrate-from-mxgraph.md (1)
195-199: LGTM! Clear migration guidance provided.The documentation effectively guides users on migrating from the removed utility methods to modern JavaScript features, maintaining consistency with the changelog.
🧰 Tools
🪛 LanguageTool
[formatting] ~196-~196: Did you mean “?”
Context: ...coalescing) and the [Optional chaining (?.)](https://developer.mozilla.org/en-US/d...(MULTIPLE_PUNCTATION_MARKS_1)
packages/core/src/util/mathUtils.ts (4)
24-24: LGTM! Import cleanup.The removal of the
getValueimport aligns with the PR objectives to remove utility functions for default value retrieval.
222-225: LGTM! Modern syntax for property access.The use of nullish coalescing operator (??) for property access is a cleaner and more idiomatic approach in modern TypeScript.
380-382: LGTM! Consistent use of modern syntax.The nullish coalescing operator (??) is used consistently for property access and default values.
616-620: LGTM! Simplified property access.The code has been modernized to use optional chaining and nullish coalescing operators, making it more concise and maintainable.
…nction # Conflicts: # CHANGELOG.md # packages/core/src/util/Utils.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
CHANGELOG.md (1)
15-15: Improve Clarity of the Removal MessageThe message on line 15 explains that utility functions for default value retrieval have been removed. Consider rewording it for clarity and grammatical accuracy. For example, changing "retrieve default value" to "retrieve default values" and simplifying the phrasing could improve readability.
Proposed diff:
- Some utility functions formerly used to retrieve default value of `CellStateStyle` and `CellStyle` properties, mainly intended to only be used internally, have been removed. + Some utility functions formerly used to retrieve default values for `CellStateStyle` and `CellStyle` properties, which were intended only for internal use, have been removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)packages/core/src/types.ts(1 hunks)packages/core/src/util/Utils.ts(1 hunks)packages/core/src/view/geometry/node/StencilShape.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/view/geometry/node/StencilShape.ts
- packages/core/src/util/Utils.ts
- packages/core/src/types.ts
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[formatting] ~17-~17: Did you mean “?”
Context: ...cing) and the [Optional chaining (?.)](https://developer.mozilla.org/en-US/d...
(MULTIPLE_PUNCTATION_MARKS_1)
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
19-19: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
21-21: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (windows-2022)
|
BREAKING CHANGES: Some utility functions formerly used to retrieve default value of
CellStateStyleandCellStyleproperties,mainly intended to only be used internally, have been removed.
Use the nullish coalescing operator (??) and the optional chaining (?.) instead.
Removed functions:
utils.getValuestringUtils.getColorstringUtils.getNumberstringUtils.getStringValueNotes
No real impact on the size of applications
Summary by CodeRabbit