perf: added menu config ts type #246
Conversation
🦋 Changeset detectedLatest commit: a06b224 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe changes introduce TypeScript type definitions to enhance type safety and developer experience in the Wangeditor project. This update affects three packages: Changes
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
packages/core/src/config/register.ts (1)
17-17: Good improvement in code clarity.Adding braces to the if statement enhances readability and reduces the risk of future errors when modifying this code. The logic remains unchanged, which is good.
For consistency with TypeScript best practices, consider using a more explicit null check:
-if (config == null) { return } +if (config === null || config === undefined) { return }This change makes the null check more explicit and aligns with TypeScript's strict null checking feature.
packages/core/src/editor/plugins/with-config.ts (1)
18-20: LGTM: Improved for...in loop implementation.The change from
lettoconstfor the loop variable is a good practice. The ESLint disable comment indicates that the lack of a guard in the for...in loop is intentional.Consider adding a comment explaining why the guard-for-in rule is disabled here, to provide context for future maintainers.
packages/editor/src/Boot.ts (3)
10-14: LGTM! Consider grouping related imports.The new imports enhance type safety for HTML parsing and generation. The reordering improves readability. However, consider grouping related imports together for better organization.
You could group the HTML-related interfaces together:
import { IDomEditor, IEditorConfig, IElemToHtmlConf, IModuleConf, IParseElemHtmlConf, IPreParseHtmlConf, // ... other imports } from '@wangeditor-next/core'Also applies to: 19-19, 21-23, 26-31, 33-34
91-92: LGTM! Improved type safety. Consider addressing the TODO comment.The updated method signature using
ISingleMenuConfiginstead of a generic object improves type safety and aligns with the new import. This change enhances the developer experience by providing more specific type information.Consider addressing the TODO comment about passing configuration during registration. If this feature is now implemented with
ISingleMenuConfig, update or remove the comment. If it's still pending, consider creating an issue to track this enhancement.
49-50: Consider addressing static analysis warnings about 'this' usage.The static analysis tool flags the use of 'this' in static methods. While these warnings are generally valid, in this context, 'this' refers to the class itself, which is a common pattern in TypeScript for accessing static properties within static methods.
To improve code clarity and silence these warnings, you could consider using the class name explicitly:
static setEditorConfig(newConfig: Partial<IEditorConfig> = {}) { Boot.editorConfig = { ...Boot.editorConfig, ...newConfig, } }Alternatively, if you prefer to keep using 'this' for flexibility, you could add a comment to explain the usage or configure the linter to ignore this specific case.
Also applies to: 58-59, 68-69, 89-90
🧰 Tools
🪛 Biome
[error] 49-49: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/core/src/config/interface.ts (2)
92-95: Review the necessity of the emptyIUploadVideoConfiginterfaceThe
IUploadVideoConfiginterface currently doesn't add any new properties toIUploadConfig. If there are no additional configurations specific to video uploads, you could useIUploadConfigdirectly or add new properties if needed.If the interface is not adding new properties, consider simplifying:
-interface IUploadVideoConfig extends IUploadConfig { - // 视频专属配置 -} +// Removed empty IUploadVideoConfig as it doesn't introduce new propertiesAlternatively, if future extensions are anticipated, it's acceptable to keep the interface as is for better code organization and readability.
96-99: Ensure consistency betweenIUploadVideoConfigandIUploadImageConfigUnlike
IUploadVideoConfig, theIUploadImageConfiginterface adds a new optional propertybase64LimitSize. For consistency, ifIUploadVideoConfigmight have unique configurations in the future, it's acceptable to retain it. Otherwise, consider aligning both interfaces for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- .changeset/spicy-foxes-add.md (1 hunks)
- packages/basic-modules/src/modules/image/helper.ts (1 hunks)
- packages/basic-modules/src/modules/link/menu/config.ts (1 hunks)
- packages/core/src/config/index.ts (2 hunks)
- packages/core/src/config/interface.ts (3 hunks)
- packages/core/src/config/register.ts (1 hunks)
- packages/core/src/editor/interface.ts (2 hunks)
- packages/core/src/editor/plugins/with-config.ts (2 hunks)
- packages/core/src/menus/interface.ts (2 hunks)
- packages/core/src/menus/register.ts (2 hunks)
- packages/editor/src/Boot.ts (3 hunks)
🧰 Additional context used
🪛 Biome
packages/editor/src/Boot.ts
[error] 49-49: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 58-58: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 59-59: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 69-69: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 89-90: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
🔇 Additional comments (25)
.changeset/spicy-foxes-add.md (1)
1-7: LGTM! Consider enhancing the commit message.The changeset file is correctly structured and the patch version bumps are appropriate for adding TypeScript types. However, the commit message could be more descriptive.
Consider expanding the commit message to provide more context. For example:
perf: add TypeScript types for menu configuration - Improves type safety and developer experience - Affects basic-modules, core, and editor packagesCould you provide more information about the impact of these changes? For instance:
- How do these new types improve the developer experience?
- Are there any potential breaking changes despite the patch version bump?
- Have you updated the documentation to reflect these new types?
packages/core/src/config/register.ts (2)
9-9: Excellent use ofPartial<T>type!The change from
IMenuConfigtoPartial<IMenuConfig>forGLOBAL_MENU_CONFis a great improvement. This allows for more flexible menu configurations, as users can now provide only a subset of the menu configuration options. This change enhances customization possibilities while maintaining backward compatibility with existing code.
Line range hint
1-19: Overall, excellent improvements to type safety and code clarity.The changes in this file enhance the flexibility of menu configurations and improve code readability. These modifications align well with TypeScript best practices and the PR objectives. The use of
Partial<IMenuConfig>and the addition of braces to the if statement are particularly commendable. Great job!packages/core/src/menus/register.ts (1)
6-8: LGTM: Import changes look goodThe addition of
ISingleMenuConfigimport and the reordering of existing imports improve code organization and prepare for the upcoming type changes in the function signature.packages/core/src/editor/plugins/with-config.ts (4)
7-11: LGTM: Import statements reorganized and new types imported.The changes in the import statements improve code organization and readability. The new import for
getMenuConfigReturnTypeis necessary for the updatedgetMenuConfigmethod signature.
29-30: LGTM: Improved if statement formatting.The addition of curly braces to the if statement enhances readability and reduces the risk of errors in future modifications. This is a good practice for maintaining consistent code style.
44-45: LGTM: Consistent improvement in if statement formatting.The addition of curly braces to the if statement in the
alertmethod is consistent with the earlier change in thegetConfigmethod. This enhances readability and maintains a consistent code style throughout the file.
35-37: LGTM: Improved type safety in getMenuConfig method.The updated method signature with the generic type parameter
K extends stringand return typegetMenuConfigReturnType<K>enhances type safety, aligning with the PR objective of improving TypeScript type definitions.To ensure this change doesn't break existing code, let's verify its usage across the codebase:
packages/core/src/config/index.ts (4)
7-8: LGTM: Import statement reorganizationThe relocation of the
forEachimport statement improves code organization by grouping similar imports together. This change doesn't affect functionality and enhances readability.
21-21: LGTM: Improved code readabilityThe addition of an empty line after the destructuring of
userConfigenhances code readability by clearly separating logical blocks of code. This is a good practice for maintaining clean and easily understandable code.
Line range hint
1-62: Summary: Approved changes with verification suggestionThe changes in this file improve code organization, flexibility, and readability:
- The import statement reorganization enhances code structure.
- The type change from
IMenuConfigtoPartial<IMenuConfig>fornewMenuConfallows for more flexible configuration.- The addition of a new line improves code readability.
These changes are approved, but please ensure to verify the usage of
newMenuConfthroughout the codebase to prevent any potential type mismatches due to the change toPartial<IMenuConfig>.
17-17: Approve type change with verification suggestionThe change from
IMenuConfigtoPartial<IMenuConfig>fornewMenuConfallows for a more flexible configuration structure. This is consistent with theuserConfigparameter type and improves the function's flexibility.To ensure this change doesn't introduce any issues, please verify the usage of
newMenuConfin other parts of the codebase. Run the following script to check for any potential type mismatches:✅ Verification successful
Type change verified successfully
The usage of
newMenuConfandIMenuConfigacross the codebase is consistent with the change toPartial<IMenuConfig>. No type mismatches were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of newMenuConf to ensure type compatibility # Search for usage of newMenuConf rg --type typescript -A 5 'newMenuConf' # Search for IMenuConfig usage to ensure it's not expecting a full implementation rg --type typescript -A 5 'IMenuConfig'Length of output: 149
Script:
#!/bin/bash # Description: Check for usage of newMenuConf to ensure type compatibility # Search for usage of newMenuConf in TypeScript files rg 'newMenuConf' --glob '*.ts' --glob '*.tsx' -A 5 # Search for IMenuConfig usage in TypeScript files to ensure it's not expecting a full implementation rg 'IMenuConfig' --glob '*.ts' --glob '*.tsx' -A 5Length of output: 4149
packages/core/src/menus/interface.ts (2)
8-8: LGTM: Import statement added forISingleMenuConfigThe import statement for
ISingleMenuConfighas been correctly added. This is necessary for the type change in theIRegisterMenuConfinterface and maintains good code organization.
77-77: Improved type safety forconfigpropertyThe change from
{ [key: string]: any }toISingleMenuConfigfor theconfigproperty type in theIRegisterMenuConfinterface enhances type safety and aligns with the PR objective. This modification will provide better IntelliSense support and catch potential type-related errors at compile-time.To ensure this change doesn't break existing implementations, please run the following script to check for any usage of
IRegisterMenuConfthat might need updating:Review the output to ensure all usages of
IRegisterMenuConfandregisterMenuare compatible with the newISingleMenuConfigtype.✅ Verification successful
Verification Successful
All usages of
IRegisterMenuConfhave been updated to useISingleMenuConfig, ensuring enhanced type safety and consistency across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of IRegisterMenuConf and check if they need updating # Search for IRegisterMenuConf usage echo "Searching for IRegisterMenuConf usage:" rg --type typescript "IRegisterMenuConf" -C 3 # Search for registerMenu function calls, which likely use this interface echo "\nSearching for registerMenu function calls:" rg --type typescript "registerMenu\(" -C 3Length of output: 346
Script:
#!/bin/bash # Description: Find usages of IRegisterMenuConf and check if they need updating # Search for IRegisterMenuConf usage in .ts and .tsx files echo "Searching for IRegisterMenuConf usage in TypeScript files:" rg "IRegisterMenuConf" --type-add 'ts=*.ts' --type-add 'tsx=*.tsx' -t ts -t tsx -C 3 # Search for registerMenu function calls in .ts and .tsx files echo "\nSearching for registerMenu function calls in TypeScript files:" rg "registerMenu\(" --type-add 'ts=*.ts' --type-add 'tsx=*.tsx' -t ts -t tsx -C 3Length of output: 554
Script:
#!/bin/bash # Description: Find usages of IRegisterMenuConf and check if they need updating # Define custom types correctly and search for IRegisterMenuConf usage echo "Searching for IRegisterMenuConf usage in TypeScript files:" rg "IRegisterMenuConf" --type-add 'typescript=*.ts,*.tsx' -t typescript -C 3 # Define custom types correctly and search for registerMenu function calls echo "\nSearching for registerMenu function calls in TypeScript files:" rg "registerMenu\(" --type-add 'typescript=*.ts,*.tsx' -t typescript -C 3Length of output: 538
Script:
#!/bin/bash # Description: Find usages of IRegisterMenuConf and registerMenu in TypeScript files # Search for IRegisterMenuConf usage in .ts and .tsx files echo "Searching for IRegisterMenuConf usage in TypeScript files:" rg "IRegisterMenuConf" -g "*.ts" -g "*.tsx" -C 3 # Search for registerMenu function calls in .ts and .tsx files echo "\nSearching for registerMenu function calls in TypeScript files:" rg "registerMenu\(" -g "*.ts" -g "*.tsx" -C 3Length of output: 6878
packages/core/src/editor/interface.ts (3)
7-13: LGTM: Import statements are well-organized.The grouping of imports from
slateand../config/interfaceimproves code readability and follows best practices for organizing imports.
19-20: Excellent addition ofgetMenuConfigReturnType<K>type alias.This new type alias enhances type safety and flexibility for the
getMenuConfigmethod. It allows for more specific return types based on the menu key, which can improve developer experience and catch potential type-related errors early in development.
31-31: Great improvement togetMenuConfigmethod signature.The updated signature for
getMenuConfigleverages the newgetMenuConfigReturnType<K>type alias, providing enhanced type safety and flexibility. This change allows for more specific return types based on the provided menu key, which can lead to better type inference and fewer runtime errors.packages/editor/src/Boot.ts (4)
46-46: LGTM! Improved readability.The added blank lines improve the visual separation between different sections of the class, enhancing readability.
Also applies to: 55-55
63-65: LGTM! Consistent formatting.The added blank lines for toolbarConfig and simpleToolbarConfig are consistent with the previous changes, further improving the overall readability of the class.
Also applies to: 72-72, 74-74
84-84: LGTM! Consistent formatting maintained.The added blank line after the plugins declaration maintains the consistent formatting throughout the class.
Line range hint
1-138: Overall, great improvements to type safety and readability!The changes in this file align well with the PR objectives of enhancing TypeScript type definitions. The introduction of specific interfaces for HTML parsing and generation, along with the use of
ISingleMenuConfig, significantly improves type safety. The consistent formatting changes enhance code readability.A few minor suggestions were made:
- Consider grouping related imports for better organization.
- Address the TODO comment in the
registerMenumethod.- Consider addressing the static analysis warnings about 'this' usage in static methods.
These changes provide a solid foundation for improved developer experience and code maintainability.
🧰 Tools
🪛 Biome
[error] 49-49: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 50-50: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 58-58: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 59-59: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 68-68: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 69-69: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 77-77: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
packages/basic-modules/src/modules/image/helper.ts (2)
158-158: Approved: Enhanced type safety foronUpdatedImagecallbackThe addition of the
as ImageElementtype assertion toimageNodeis a good practice. It ensures type safety when passing the node to theonUpdatedImagecallback, which can help prevent potential runtime errors and improve code reliability. This change aligns well with TypeScript best practices and the overall goal of improving type definitions in the project.
Line range hint
1-158: Overall assessment: Improved type safety in image helper functionsThe change in this file is minimal but effective. It enhances type safety for the
updateImageNodefunction's callback, which is in line with the PR's objective of improving TypeScript type definitions. The rest of the file remains unchanged, maintaining its existing functionality for image-related operations in the editor.packages/core/src/config/interface.ts (2)
105-173: Improved type safety with explicit definitions inIMenuConfigDefining explicit properties for each menu item in the
IMenuConfiginterface enhances type safety and clarity. This change reduces the risk of errors due to misspelled keys and improves the developer experience by providing clear interfaces.
204-204: Flexible menu configuration withPartial<IMenuConfig>Updating
MENU_CONFto be of typePartial<IMenuConfig>allows users to provide only the configurations they need to customize, making the configuration process more flexible and user-friendly.
| [key: string]: any; | ||
| iconSvg?: string; |
There was a problem hiding this comment.
Consider specifying more precise types instead of 'any' in ISingleMenuConfig
The use of [key: string]: any; in the ISingleMenuConfig interface reduces type safety and can lead to unintended property assignments without type checking. It's recommended to define specific properties or use a generic type parameter to enhance type safety and provide better code completion and validation.
To enhance type safety, consider removing the index signature or replacing any with a more specific type:
interface ISingleMenuConfig {
- [key: string]: any;
iconSvg?: string;
}If dynamic keys are necessary, you might define a generic interface or use a more restrictive index signature.
Committable suggestion was skipped due to low confidence.
Changes Overview
Implementation Approach
Testing Done
Verification Steps
Additional Notes
Checklist
Related Issues
Summary by CodeRabbit
New Features
Bug Fixes
Documentation