codesandbox[bot]
commented
2 months ago Review or Edit in CodeSandbox
Open the branch in Web Editor • VS Code • Insiders
Open Preview
Closed cheton closed 2 months ago
codesandbox[bot]
commented
2 months ago Open the branch in Web Editor • VS Code • Insiders
Open Preview
changeset-bot[bot]
commented
2 months ago Latest commit: 831ac6977a6e0cbb01f710ca07f36cd5992f4c19
The changes in this PR will be included in the next version bump.
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
codesandbox-ci[bot]
commented
2 months ago This pull request is automatically built and testable in CodeSandbox.
To see build info of the built libraries, click here or the icon next to each commit SHA.
trendmicro-frontend-bot
commented
2 months ago On 2024-09-19 02:06:14 +0000, PR #922 (831ac6977a6e0cbb01f710ca07f36cd5992f4c19) was successfully deployed. You can view it at the following link: https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-922/
codecov[bot]
commented
2 months ago Attention: Patch coverage is 84.39153% with 59 lines in your changes missing coverage. Please review.
Project coverage is 77.26%. Comparing base (
e42456d) to head (831ac69). Report is 1 commits behind head on v2.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
codiumai-pr-agent-free[bot]
commented
2 months ago | ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪ |
| 🧪 PR contains tests |
| 🔒 No security concerns identified |
| ⚡ Key issues to review Performance Concern The `resolveProps` function uses nested loops and object spread operations, which may impact performance for components with many props or deeply nested structures. Consider optimizing this function for better performance. Potential Memory Leak The `cachedIconRef` is used to cache icons, but there's no mechanism to clear or limit the cache size. This could lead to memory issues if many different icons are used over time. Redundant Theme Configuration The theme configuration is defined twice, once at the top level and once inside the `App` component. This could lead to confusion and maintenance issues. Consider consolidating these configurations. |
cheton
commented
2 months ago /review
codiumai-pr-agent-free[bot]
commented
2 months ago cheton
commented
2 months ago /improve
cheton
commented
2 months ago /improve
codiumai-pr-agent-free[bot]
commented
2 months ago Latest suggestions up to 655d53f
| Category | Suggestion | Score |
| Enhancement |
Replace Object.prototype.hasOwnProperty.call() with Object.hasOwn() for improved performance and readability___ **Consider usingObject.hasOwn() instead of Object.prototype.hasOwnProperty.call() for better performance and readability.** [packages/react/src/default-props/resolveProps.js [28-31]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-53301b65a7bef0cc35b47b0f8ba99680614b01378a1e0da0688800746fd01095R28-R31) ```diff -if (Object.prototype.hasOwnProperty.call(defaultSlotProps, slotKey)) { +if (Object.hasOwn(defaultSlotProps, slotKey)) { const slotPropName = slotKey; output[propName][slotPropName] = resolveProps(defaultSlotProps[slotPropName], slotProps[slotPropName]); } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 9Why: The suggestion to use Object.hasOwn() is valid as it improves both performance and readability. This change is directly applicable to the code and aligns with modern JavaScript practices. | 9 |
Use object destructuring to improve code readability and reduce variable assignments___ **Consider using object destructuring for thecheckboxGroupContext to improve code readability and reduce the number of variable assignments.** [packages/react/src/checkbox/Checkbox.js [45-54]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-d5ae55a366f7b950201c3eed96f52580aac6a13b4d1ff587e33bc597a67a5c51R45-R54) ```diff if (checkboxGroupContext) { - checked = checkboxGroupContext.value.includes(value); - disabled = disabled ?? checkboxGroupContext.disabled; - name = name ?? checkboxGroupContext.name; - onChange = callAll(checkboxGroupContext.onChange, onChange); - size = size ?? checkboxGroupContext.size; - variantColor = variantColor ?? checkboxGroupContext.variantColor; + const { value: groupValue, disabled: groupDisabled, name: groupName, onChange: groupOnChange, size: groupSize, variantColor: groupVariantColor } = checkboxGroupContext; + checked = groupValue.includes(value); + disabled = disabled ?? groupDisabled; + name = name ?? groupName; + onChange = callAll(groupOnChange, onChange); + size = size ?? groupSize; + variantColor = variantColor ?? groupVariantColor; } else { size = size ?? defaultSize; variantColor = variantColor ?? defaultVariantColor; } ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: This suggestion significantly improves code readability and maintainability by reducing repetitive variable assignments. | 8 | |
Extract date-related logic into a custom hook for better organization and reusability___ **Consider using a custom hook to handle date-related logic and state management toimprove code organization and reusability.** [packages/react/src/date-pickers/Calendar/Calendar.js [63-67]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-36d3b4ebdd3a539abc36a7c46f957d32265330a2fb81f6a576d4f0c11017b967R63-R67) ```diff -const initialDate = useConst(() => { - const value = dateProp ?? defaultDateProp; - return mapValueToDate(value); -}); -const [date, setDate] = useState(initialDate); -const [month, setMonth] = useState(initialDate ?? new Date()); +const { date, setDate, month, setMonth } = useDateState(dateProp, defaultDateProp); +// ... (implement useDateState hook in a separate file) + ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: The suggestion enhances code organization and reusability, but it requires additional implementation effort for the custom hook. | 7 | |
Simplify context usage by leveraging the spread operator and default values___ **Consider using the spread operator to assign default values from the radio groupcontext, which would simplify the code and make it more maintainable.** [packages/react/src/radio/Radio.js [32-37]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-902e8001f5f749a8725aa09ca99a7020fb3923fca8861a8478b575090d353573R32-R37) ```diff -let checked = checkedProp; -let disabled = disabledProp; -let name = nameProp; -let onChange = onChangeProp; -let size = sizeProp; -let variantColor = variantColorProp; +const radioGroupContext = useRadioGroup(); +const { + checked = checkedProp, + disabled = disabledProp, + name = nameProp, + onChange = onChangeProp, + size = sizeProp ?? defaultSize, + variantColor = variantColorProp ?? defaultVariantColor, +} = radioGroupContext ?? {}; ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: This suggestion improves code maintainability and readability by consolidating default value assignments using the spread operator, which is a beneficial enhancement for handling context values. | 7 | |
Use object destructuring for improved code clarity___ **Consider using object destructuring forinProps to improve code readability and make it clear which props are being used.** [packages/react/src/menu/MenuDivider.js [7]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-7df9cf6b0636731e10927e344d036555051ed88002560cdc218a55403bed8231R7-R7) ```diff -const props = useDefaultProps({ props: inProps, name: 'MenuDivider' }); +const { ...props } = useDefaultProps({ props: inProps, name: 'MenuDivider' }); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 3Why: The suggestion to use object destructuring is a minor enhancement for code readability, but it does not significantly impact functionality or performance. | 3 | |
| Possible issue |
Add a safety check for the existence of theme components before accessing them___ **Consider adding a check fortheme?.components before accessing it to prevent potential errors if the theme object is not properly structured.** [packages/react/src/default-props/useDefaultProps.js [37-41]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-9e2b08718529118de37fa2be5ac0db9b273da8acad370e183c598387a0da497fR37-R41) ```diff const theme = { components: context, }; +if (!theme.components) { + return props; +} + return getThemeProps({ props, name, theme }); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 8Why: Adding a check for theme?.components enhances the robustness of the code by preventing potential runtime errors if the theme object is not structured as expected. | 8 |
| Maintainability |
Move theme configuration to a separate file or function for better organization___ **Consider moving the theme configuration to a separate file or function to improvecode organization and maintainability.** [packages/react-docs/pages/_app.page.js [36-47]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-49f34385f9684300c85addd1b7dc358db1cfc469e159754c0ea81c147fa99672R36-R47) ```diff -theme.components = { - // Set default props for components here. - // - // Example: - // ``` - // 'AccordionToggle': { - // defaultProps: { - // disabled: true, - // }, - // } - // ``` -}; +import { getThemeConfig } from '../utils/themeConfig'; +// ... + +theme.components = getThemeConfig(); + ``` Suggestion importance[1-10]: 7Why: This suggestion improves code organization and maintainability by separating concerns, but it is not crucial for functionality. | 7 |
| Performance |
Memoize the getIconFromTheme function to optimize performance___ **Consider memoizing thegetIconFromTheme function to avoid unnecessary re-creation on each render.** [packages/react/src/icon/Icon.js [18-35]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-a6f3bf404c157f222fdec232e3605920c8ad56430b97094c7b441c1b5335973bR18-R35) ```diff -const getIconFromTheme = (iconKey) => { +const getIconFromTheme = React.useCallback((iconKey) => { const [cachedKey, cachedIcon] = cachedIconRef.current; // Return the cached icon if it matches the current key if (cachedKey === iconKey) { return cachedIcon; } // Find the icon in the theme's icon set const foundIcon = ensureArray(theme?.icons).find(([key]) => key === iconKey)?.[1]; // Cache the found icon if it exists if (foundIcon) { cachedIconRef.current = [iconKey, foundIcon]; } return foundIcon; -}; +}, [theme]); ``` - [ ] **Apply this suggestion** Suggestion importance[1-10]: 7Why: Memoizing the getIconFromTheme function can improve performance by avoiding unnecessary re-creations on each render. This is a good optimization, though not critical. | 7 |
Memoize event handler functions using useCallback to optimize performance___ **Consider using theuseCallback hook for the handleBlur, handleFocus, and handleChange functions to optimize performance by preventing unnecessary re-renders.**
[packages/react/src/input/InputControl.js [56-75]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-77d54bf2410b8b5ed6a309ea56ef4f7b6e4646999f20f20ab3a9f64a567454e5R56-R75)
```diff
-const handleBlur = (event) => {
+const handleBlur = useCallback((event) => {
setFocused(false);
if (onBlur) {
onBlur(event);
}
-};
-const handleFocus = (event) => {
+}, [onBlur]);
+
+const handleFocus = useCallback((event) => {
setFocused(true);
if (onFocus) {
onFocus(event);
}
-};
-const handleChange = (event) => {
+}, [onFocus]);
+
+const handleChange = useCallback((event) => {
if (!isControlled) {
setValue(event.target.value);
}
if (onChange) {
onChange(event, event.target.value);
}
-};
+}, [isControlled, onChange]);
```
- [ ] **Apply this suggestion**
Suggestion importance[1-10]: 7Why: Using useCallback for event handlers can prevent unnecessary re-renders, optimizing performance. This is a beneficial change, though its impact may be minor depending on the component's usage. | 7 | |
| Best practice |
Use a constant for configuration values to improve consistency and maintainability___ **Consider using a constant for theuseCSSVariables configuration to improve code consistency and maintainability.** [packages/react-docs/pages/_app.page.js [84-88]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-49f34385f9684300c85addd1b7dc358db1cfc469e159754c0ea81c147fa99672R84-R88) ```diff +const USE_CSS_VARIABLES = true; + +// ... + config: { ...theme?.config, // Enable CSS variables replacement - useCSSVariables: true, + useCSSVariables: USE_CSS_VARIABLES, }, ``` Suggestion importance[1-10]: 6Why: Using a constant for configuration values enhances consistency and maintainability, but the impact is minor. | 6 |
| Category | Suggestion | Score |
| Enhancement |
Add PropTypes validation for component props___ **Add PropTypes validation for thevalue and children props to ensure proper usage of the component.** [packages/react/src/default-props/DefaultPropsProvider.js [4-13]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-4443360626424d916b7b15728905151d5f2ea853fe6b47908133968f1f9a6015R4-R13) ```diff +import PropTypes from 'prop-types'; + const DefaultPropsProvider = ({ value, children, }) => { return ( Suggestion importance[1-10]: 7Why: | 7 |
✅ Add a test case for nested object merging in slotProps___Suggestion Impact:The commit added a test case for merging `slots` and `slotProps` with nested objects, similar to the suggestion. code diff: ```diff it('merge `slots` and `slotProps` props', () => { - expect( - resolveProps( - { - slots: { - input: 'input', - }, - slotProps: { - input: { - className: 'input', - }, + const defaultProps = { + slots: { + input: 'input', + }, + slotProps: { + input: { + className: 'input', + style: { + fontSize: 'sm', }, }, - { - slots: { - root: 'root', - }, - slotProps: { - root: { - className: 'root', - }, - input: { - style: { - color: 'red', - }, - }, + }, + }; + const props = { + slots: { + root: 'root', + }, + slotProps: { + root: { + className: 'root', + }, + input: { + style: { + color: 'red', }, }, - ), - ).toEqual({ + }, + }; + + expect(resolveProps(defaultProps, props)).toEqual({ slots: { root: 'root', input: 'input', ```resolveProps function correctly handles nested objects in the slotProps.**
[packages/react/src/default-props/__tests__/resolveProps.test.js [47-92]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-7617e3a17d9989c2165f8f3246cc7aca87ed516ce079ef0db72a611ab6340e62R47-R92)
```diff
it('merge `slots` and `slotProps` props', () => {
expect(
resolveProps(
{
slots: {
input: 'input',
},
slotProps: {
input: {
className: 'input',
+ style: {
+ fontSize: '14px',
+ },
},
},
},
{
slots: {
root: 'root',
},
slotProps: {
root: {
className: 'root',
},
input: {
style: {
color: 'red',
+ fontWeight: 'bold',
},
},
},
},
),
).toEqual({
slots: {
root: 'root',
input: 'input',
},
slotProps: {
root: {
className: 'root',
},
input: {
className: 'input',
style: {
+ fontSize: '14px',
color: 'red',
+ fontWeight: 'bold',
},
},
},
});
});
```
Suggestion importance[1-10]: 7Why: | 7 | |
✅ Add a default value to the context___Suggestion Impact:The commit implemented the suggestion by adding a default value of an empty object to the DefaultPropsContext. code diff: ```diff -const DefaultPropsContext = createContext(); +const DefaultPropsContext = createContext({}); ```DefaultPropsContext to provide a fallback when the context is not wrapped with a provider.** [packages/react/src/default-props/context.js [3]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-89d85520da03a3bbe12cbccd7beb12d8f85b444abdd7ad817f0245603ac7e891R3-R3) ```diff -const DefaultPropsContext = createContext(); +const DefaultPropsContext = createContext({}); ``` Suggestion importance[1-10]: 7Why: | 7 | |
✅ Use Object.entries() for cleaner object iteration___Suggestion Impact:The suggestion to use Object.entries() instead of a for...in loop was implemented, making the code more concise and eliminating the need for the hasOwnProperty check. code diff: ```diff - for (const key in defaultProps) { - if (!Object.prototype.hasOwnProperty.call(defaultProps, key)) { - continue; - } - - const propName = key; - + for (const [propName, defaultValue] of Object.entries(defaultProps)) { ```Object.entries() instead of a for...in loop to iterate over the defaultProps object. This can make the code more concise and avoid the need for the hasOwnProperty check.**
[packages/react/src/default-props/resolveProps.js [10-15]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-53301b65a7bef0cc35b47b0f8ba99680614b01378a1e0da0688800746fd01095R10-R15)
```diff
-for (const key in defaultProps) {
- if (!Object.prototype.hasOwnProperty.call(defaultProps, key)) {
- continue;
- }
+for (const [propName, defaultValue] of Object.entries(defaultProps)) {
- const propName = key;
-
```
Suggestion importance[1-10]: 7Why: | 7 | |
Simplify conditional logic using modern JavaScript features___ **Consider using optional chaining (?.) and nullish coalescing (??) operators to simplify the nested conditional checks for config?.defaultProps and config?.styleOverrides.**
[packages/react/src/default-props/useDefaultProps.js [12-19]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-9e2b08718529118de37fa2be5ac0db9b273da8acad370e183c598387a0da497fR12-R19)
```diff
if (config?.defaultProps) {
return resolveProps(config.defaultProps, props);
}
-if (!(config?.styleOverrides) && !(config?.variants)) {
- // no property 'defaultProps'
- return resolveProps(config, props);
-}
+return (config?.styleOverrides || config?.variants)
+ ? props
+ : resolveProps(config ?? {}, props);
```
Suggestion importance[1-10]: 7Why: | 7 | |
Use nullish coalescing for more precise fallback logic___ **Consider using the nullish coalescing operator (??) instead of the logical OR (||) when checking for foundIcon. This will only fall back to children if foundIcon is null or undefined, not for other falsy values like an empty string.**
[packages/react/src/icon/Icon.js [37-39]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-a6f3bf404c157f222fdec232e3605920c8ad56430b97094c7b441c1b5335973bR37-R39)
```diff
const icon = (typeof iconProp === 'string' && iconProp.length > 0)
- ? getIconFromTheme(iconProp) || children
+ ? getIconFromTheme(iconProp) ?? children
: children;
```
Suggestion importance[1-10]: 7Why: | 7 | |
Simplify the component's parameter destructuring to improve code readability___ **Consider using object destructuring forinProps directly in the function parameters to improve readability and reduce the need for an additional destructuring statement.** [packages/react/src/menu/MenuToggle.js [12-19]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-0e71eac30ffa43dd0393c18e52a244932d12c8ca6198ac74b056427f7ede41cfR12-R19) ```diff -const MenuToggle = forwardRef((inProps, ref) => { - const { - children, - disabled, - onClick: onClickProp, - onKeyDown: onKeyDownProp, - ...rest - } = useDefaultProps({ props: inProps, name: 'MenuToggle' }); +const MenuToggle = forwardRef(({ children, disabled, onClick: onClickProp, onKeyDown: onKeyDownProp, ...inProps }, ref) => { + const { ...rest } = useDefaultProps({ props: inProps, name: 'MenuToggle' }); ``` Suggestion importance[1-10]: 7Why: | 7 | |
Extract modal logic into a custom hook to improve component organization and reusability___ **Consider using a custom hook to handle the modal's open/close logic and statemanagement, which can simplify the component and improve reusability.** [packages/react/src/modal/Modal.js [16-33]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-49f8cae28ebb8681d3656a307d07bd9782ac654092cbdc89fa092a91b1d6591fR16-R33) ```diff -const Modal = forwardRef((inProps, ref) => { +const useModal = (inProps) => { const { autoFocus = false, - children, closeOnEsc = false, closeOnOutsideClick = false, finalFocusRef, initialFocusRef, isOpen, onClose, onEsc, onOverlayClick, scrollBehavior = defaultScrollBehavior, size = defaultSize, - ...rest } = useDefaultProps({ props: inProps, name: 'Modal' }); + + // Add logic for handling open/close, focus management, etc. + + return { + isOpen, + onClose, + // ... other necessary properties and methods + }; +}; +const Modal = forwardRef((inProps, ref) => { + const { children, ...rest } = inProps; + const modalProps = useModal(rest); + + return ( + // Render modal using modalProps + ); +}); + ``` Suggestion importance[1-10]: 7Why: | 7 | |
Use nullish coalescing operator for default values to handle falsy values correctly___ **Consider using the nullish coalescing operator (??) instead of logical OR (||) fordefault values to avoid issues with falsy values like empty string or zero.** [packages/react/src/radio/Radio.js [32-37]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-902e8001f5f749a8725aa09ca99a7020fb3923fca8861a8478b575090d353573R32-R37) ```diff -let checked = checkedProp; -let disabled = disabledProp; -let name = nameProp; -let onChange = onChangeProp; -let size = sizeProp; -let variantColor = variantColorProp; +let checked = checkedProp ?? undefined; +let disabled = disabledProp ?? false; +let name = nameProp ?? undefined; +let onChange = onChangeProp ?? undefined; +let size = sizeProp ?? defaultSize; +let variantColor = variantColorProp ?? defaultVariantColor; ``` Suggestion importance[1-10]: 7Why: | 7 | |
| Possible issue |
Use optional chaining to safely access potentially undefined context___ **Consider using the optional chaining operator (?.) when accessing menuContext properties to avoid potential errors if menuContext is undefined.**
[packages/react/src/menu/MenuContent.js [24]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-fbfbee8b32abf112fa5928ade05954ec314b3a96322d89162dfd99bce4fd99f9R24-R24)
```diff
const menuContext = useMenu(); // context might be an undefined value
+const {
+ anchorEl,
+ isOpen,
+ onClose,
+ placement,
+} = menuContext ?? {};
```
Suggestion importance[1-10]: 7Why: | 7 |
Use optional chaining to safely access properties of potentially undefined objects___ **Consider using the optional chaining operator (?.) when accessing properties ofinProps to avoid potential errors if inProps is undefined.**
[packages/react/src/pagination/Pagination.js [12-22]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-3c78d53111928b7fdfd1d6d2827395ae5c3770b91c76423fa66df50007da62b0R12-R22)
```diff
const {
children,
defaultPage = 1,
defaultValue,
disabled,
name: nameProp,
size,
value: valueProp,
variantColor,
onChange,
-} = useDefaultProps({ props: inProps, name: 'Pagination' });
+} = useDefaultProps({ props: inProps, name: 'Pagination' }) ?? {};
```
Suggestion importance[1-10]: 7Why: | 7 |
| Category | Suggestion | Score |
| Performance |
✅ Memoize the DefaultPropsProvider value to optimize rendering performance___ **Consider memoizing theDefaultPropsProvider value to prevent unnecessary re-renders of child components when the theme doesn't change.** [packages/react/src/theme/ThemeProvider.js [79-85]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-160beeffe0ffadbdf65362186091704b4ba343fcce76c7914d9954625048c064R79-R85) ```diff - Suggestion importance[1-10]: 9Why: Memoizing the DefaultPropsProvider value is an effective optimization to prevent unnecessary re-renders of child components, which can significantly improve performance in a React application. | 9 |
✅ Use Object.hasOwn() for more efficient property existence checking___Suggestion Impact:The suggestion to use Object.hasOwn() was implemented by replacing the for-in loop with Object.entries(), which inherently checks for own properties. code diff: ```diff - for (const key in defaultProps) { - if (!Object.prototype.hasOwnProperty.call(defaultProps, key)) { - continue; - } - - const propName = key; - + for (const [propName, defaultValue] of Object.entries(defaultProps)) { ```as Object.hasOwn(), which is more performant and avoids potential issues with inherited properties.** [packages/react/src/default-props/resolveProps.js [11-13]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-53301b65a7bef0cc35b47b0f8ba99680614b01378a1e0da0688800746fd01095R11-R13) ```diff -if (!Object.prototype.hasOwnProperty.call(defaultProps, key)) { +if (!Object.hasOwn(defaultProps, key)) { continue; } ``` Suggestion importance[1-10]: 7Why: Using Object.hasOwn() is a more modern and efficient way to check for property existence, which can improve performance and avoid issues with inherited properties. | 7 | |
| Error handling |
✅ Add input validation for the 'name' parameter in the useDefaultProps hook___ **Consider adding error handling for cases where thename parameter is not provided or is invalid. This can help prevent potential runtime errors and improve the robustness of the hook.** [packages/react/src/default-props/useDefaultProps.js [24-28]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-9e2b08718529118de37fa2be5ac0db9b273da8acad370e183c598387a0da497fR24-R28) ```diff const useDefaultProps = ({ props, name }) => { if (!useContext) { throw new Error('The `useContext` hook is not available with your React version.'); } + + if (!name || typeof name !== 'string') { + throw new Error('Invalid or missing component name provided to useDefaultProps'); + } ``` `[Suggestion has been applied]` Suggestion importance[1-10]: 8Why: Adding input validation for the 'name' parameter is a good practice to prevent potential runtime errors and improve the robustness of the hook, especially since the 'name' is crucial for resolving default props. | 8 |
| Best practice |
Add prop-types validation to the DefaultPropsProvider component___ **Consider adding prop-types validation for thevalue and children props to improve component reliability and provide better error messages during development.** [packages/react/src/default-props/DefaultPropsProvider.js [3-12]](https://github.com/trendmicro-frontend/tonic-ui/pull/922/files#diff-4443360626424d916b7b15728905151d5f2ea853fe6b47908133968f1f9a6015R3-R12) ```diff +import PropTypes from 'prop-types'; + const DefaultPropsProvider = ({ value, children, }) => { return ( Suggestion importance[1-10]: 6Why: Adding prop-types validation is a best practice that enhances component reliability and provides better error messages during development, although it is not critical for functionality. | 6 |
cheton
commented
2 months ago /ask I need to use a JSX parser to scan files and check if the name in useDefaultProps matches the component name. The parser should only traverse files that import the useDefaultProps hook. Keep in mind, some files may not include forwardRef, so please adjust my code so it doesn't depend on forwardRef.
Example:
const AccordionBody = forwardRef((inProps, ref) => {
const props = useDefaultProps({ props: inProps, name: 'AccordionBody' });Here is my code for reference:
const fs = require('node:fs');
const { globSync } = require('glob');
const parser = require('@babel/parser');
const traverse = require('@babel/traverse').default;
const sourceDirectory = './';
const files = globSync([
'*/**/*.js',
], {
'ignore': [
'*/**/__tests__/**/*.js',
],
}).sort();
for (const file of files) {
const code = fs.readFileSync(file, { encoding: 'utf-8' });
const ast = parser.parse(code, {
sourceType: 'module',
plugins: ['jsx'],
});
traverse(ast, {
VariableDeclarator(path) {
if (path.node.init && path.node.init.callee && path.node.init.callee.name === 'forwardRef') {
const componentName = path.node.id.name;
path.traverse({
CallExpression(innerPath) {
if (innerPath.node.callee.name === 'useDefaultProps') {
const nameProperty = innerPath.node.arguments[0].properties.find(prop => prop.key.name === 'name');
if (nameProperty && nameProperty.value.value !== componentName) {
console.log(`Mismatch found: ${nameProperty.value.value} does not match ${componentName}`);
} else {
//console.log(`Found: ${nameProperty.value.value} match ${componentName}`);
}
}
}
});
}
}
});
}
I need to use a JSX parser to scan files and check if the name in useDefaultProps matches the component name. The parser should only traverse files that import the useDefaultProps hook. Keep in mind, some files may not include forwardRef, so please adjust my code so it doesn't depend on forwardRef. Example: jsx const AccordionBody = forwardRef((inProps, ref) => { const props = useDefaultProps({ props: inProps, name: 'AccordionBody' }); Here is my code for reference: js const fs = require('node:fs'); const { globSync } = require('glob'); const parser = require('@babel/parser'); const traverse = require('@babel/traverse').default; const sourceDirectory = './'; const files = globSync([ '*/**/*.js', ], { 'ignore': [ '*/**/__tests__/**/*.js', ], }).sort(); for (const file of files) { const code = fs.readFileSync(file, { encoding: 'utf-8' }); const ast = parser.parse(code, { sourceType: 'module', plugins: ['jsx'], }); traverse(ast, { VariableDeclarator(path) { if (path.node.init && path.node.init.callee && path.node.init.callee.name === 'forwardRef') { const componentName = path.node.id.name; path.traverse({ CallExpression(innerPath) { if (innerPath.node.callee.name === 'useDefaultProps') { const nameProperty = innerPath.node.arguments[0].properties.find(prop => prop.key.name === 'name'); if (nameProperty && nameProperty.value.value !== componentName) { console.log(`Mismatch found: ${nameProperty.value.value} does not match ${componentName}`); } else { //console.log(`Found: ${nameProperty.value.value} match ${componentName}`); } } } }); } } }); }
To adjust your code so that it doesn't depend on forwardRef, you can modify the traversal logic to look for any function declaration or variable declaration that uses useDefaultProps. This way, you can check if the name argument in useDefaultProps matches the component name, regardless of whether forwardRef is used. Here's how you can do it:
const fs = require('node:fs');
const { globSync } = require('glob');
const parser = require('@babel/parser');
const traverse = require('@babel/traverse').default;
const sourceDirectory = './';
const files = globSync([
'*/**/*.js',
], {
'ignore': [
'*/**/__tests__/**/*.js',
],
}).sort();
for (const file of files) {
const code = fs.readFileSync(file, { encoding: 'utf-8' });
const ast = parser.parse(code, {
sourceType: 'module',
plugins: ['jsx'],
});
traverse(ast, {
FunctionDeclaration(path) {
const functionName = path.node.id.name;
path.traverse({
CallExpression(innerPath) {
if (innerPath.node.callee.name === 'useDefaultProps') {
const nameProperty = innerPath.node.arguments[0].properties.find(prop => prop.key.name === 'name');
if (nameProperty && nameProperty.value.value !== functionName) {
console.log(`Mismatch found: ${nameProperty.value.value} does not match ${functionName}`);
}
}
}
});
},
VariableDeclarator(path) {
if (path.node.init && (path.node.init.type === 'ArrowFunctionExpression' || path.node.init.type === 'FunctionExpression')) {
const componentName = path.node.id.name;
path.traverse({
CallExpression(innerPath) {
if (innerPath.node.callee.name === 'useDefaultProps') {
const nameProperty = innerPath.node.arguments[0].properties.find(prop => prop.key.name === 'name');
if (nameProperty && nameProperty.value.value !== componentName) {
console.log(`Mismatch found: ${nameProperty.value.value} does not match ${componentName}`);
}
}
}
});
}
}
});
}FunctionDeclaration: This handles named function declarations. It checks if useDefaultProps is called within the function and compares the name property with the function name.
VariableDeclarator: This handles arrow functions and function expressions assigned to variables. It checks if useDefaultProps is called within the function and compares the name property with the variable name.
This approach ensures that you can identify mismatches in component names and useDefaultProps names, regardless of whether forwardRef is used.
https://trendmicro-frontend.github.io/tonic-ui-demo/react/pr-922/
User description
PR Type
enhancement, tests, documentation
Description
DefaultPropsProvidercomponent anduseDefaultPropshook to manage default props in React components.DefaultPropsProviderintoThemeProviderto pass theme components.useDefaultPropstoAccordionandAlertcomponents for simplified props handling.resolvePropsfunction and corresponding tests to merge default and provided props.Changes walkthrough 📝
7 files
DefaultPropsProvider.js
Add DefaultPropsProvider component for default props contextpackages/react/src/default-props/DefaultPropsProvider.js
DefaultPropsProvidercomponent.useDefaultProps.js
Implement useDefaultProps hook for resolving propspackages/react/src/default-props/useDefaultProps.js
useDefaultPropshook.resolveProps.js
Add resolveProps function for merging propspackages/react/src/default-props/resolveProps.js
resolvePropsfunction.ThemeProvider.js
Integrate DefaultPropsProvider into ThemeProviderpackages/react/src/theme/ThemeProvider.js
DefaultPropsProviderintoThemeProvider.DefaultPropsProvider.Accordion.js
Apply useDefaultProps to Accordion componentpackages/react/src/accordion/Accordion.js
useDefaultPropstoAccordioncomponent.Alert.js
Apply useDefaultProps to Alert componentpackages/react/src/alert/Alert.js
useDefaultPropstoAlertcomponent.index.js
Export default props utilitiespackages/react/src/index.js
DefaultPropsProvideranduseDefaultProps.1 files
resolveProps.test.js
Add tests for resolveProps functionpackages/react/src/default-props/__tests__/resolveProps.test.js
resolvePropsfunction.2 files
_app.page.js
Configure theme components for default propspackages/react-docs/pages/_app.page.js
index.page.mdx
Update migration guide link for iconspackages/react-docs/pages/getting-started/icons/index.page.mdx - Updated migration guide link for icons.