MM-69835 Update usage of propTypes and defaultProps - #12
Conversation
hmhealey
changed the title
| // render, without wrapping the component (which would break ref forwarding, | ||
| // static reads, and — for e.g. `bsRole` — class `defaultProps` merging). | ||
| function patchRenderValidation(Component, propName, allowed) { | ||
| const flag = `__bsValidated_${propName}`; |
There was a problem hiding this comment.
Yeah, this is ugly, but I couldn't come up with a better solution than what was generated by Claude without going through each component and manually adding the propTypes and default values
|
@coderabbitai Review this |
|
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe change moves several prop validations from declaration time to render time. It adds shared role-error helpers, updates component warnings, changes bootstrap decorators to validate during rendering, updates tests, and makes generated-file patterns recursive. ChangesRuntime validation warnings
Generated file patterns
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dropdown
participant RoleErrorHelpers
participant warning
Dropdown->>RoleErrorHelpers: check required and duplicate roles
RoleErrorHelpers-->>Dropdown: return warning message or null
Dropdown->>warning: report invalid role configuration
sequenceDiagram
participant DecoratedComponent
participant applyBsProp
participant React
participant warning
DecoratedComponent->>applyBsProp: apply defaults and validate props
applyBsProp->>React: create wrapped element
applyBsProp->>warning: report invalid allowed values
React-->>DecoratedComponent: render element
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ProgressBar.js (1)
21-43: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard empty children before reading
child.type.React.Children.forEachinvokes its callback fornull,undefined, and booleans. When a truthy children array contains one of these values, Line 36 throws beforewarningruns. Skip empty nodes, checkReact.isValidElement(child)before readingtype, and useString(child.type)for unsupported element types. Preserve the existingelement.typecomparison to retain proxy compatibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ProgressBar.js` around lines 21 - 43, Update getInvalidChildError to skip null, undefined, and boolean children before accessing child.type, and only read type after confirming React.isValidElement(child). Preserve the existing element.type comparison for proxy compatibility, and stringify unsupported element types with String(child.type) when constructing the error message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Nav.js`:
- Around line 283-287: The warning condition in Nav should use the effective
navbar value, including the navbar state derived from navbarContext when
propsNavbar is absent, rather than checking only propsNavbar. Update the
condition near warning() and align it with the value used by the
navbar-rendering logic so justified Nav components in navbar context trigger the
unsupported-style warning.
In `@src/utils/bootstrapUtils.js`:
- Around line 49-60: Update the validator setup around validatedRender so its
mutable configuration is stored under flag rather than captured permanently in
the first allowed array. When the existing flag is found, refresh that
configuration’s allowed value before returning, so later bsSizes calls validate
against rebuilt values; add coverage using two bsSizes decorators on a class
component.
- Around line 99-118: Update curry to detect React.memo and React.forwardRef
results as supported component types instead of treating them as plain objects.
Apply bsClass, bsStyles, bsSizes, default props, and validation while preserving
each component’s memoization or ref behavior. Add React 19 coverage for all
three decorators on both exotic component types.
---
Outside diff comments:
In `@src/ProgressBar.js`:
- Around line 21-43: Update getInvalidChildError to skip null, undefined, and
boolean children before accessing child.type, and only read type after
confirming React.isValidElement(child). Preserve the existing element.type
comparison for proxy compatibility, and stringify unsupported element types with
String(child.type) when constructing the error message.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8b0cd44-f0c8-4776-b284-91acae4f62ec
📒 Files selected for processing (12)
.gitattributessrc/ButtonGroup.jssrc/Dropdown.jssrc/MenuItem.jssrc/Nav.jssrc/ProgressBar.jssrc/utils/PropTypes.jssrc/utils/bootstrapUtils.jstest/DropdownSpec.jstest/ProgressBarSpec.jstest/index.jstest/utils/bootstrapUtilsSpec.js
💤 Files with no reviewable changes (1)
- test/index.js
| warning( | ||
| !(justified && propsNavbar), | ||
| 'justified navbar `Nav`s are not supported' | ||
| ); | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the effective navbar value for the warning.
Line 294 enables navbar rendering from navbarContext when propsNavbar is absent. Line 284 only checks propsNavbar. A justified Nav in navbar context therefore receives navbar styling without the development warning.
Proposed fix
+ const navbar = propsNavbar != null ? propsNavbar : navbarContext;
+
warning(
- !(justified && propsNavbar),
+ !(justified && navbar),
'justified navbar `Nav`s are not supported'
);
...
- const navbar = propsNavbar != null ? propsNavbar : navbarContext;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Nav.js` around lines 283 - 287, The warning condition in Nav should use
the effective navbar value, including the navbar state derived from
navbarContext when propsNavbar is absent, rather than checking only propsNavbar.
Update the condition near warning() and align it with the value used by the
navbar-rendering logic so justified Nav components in navbar context trigger the
unsupported-style warning.
| if (Object.prototype.hasOwnProperty.call(proto, flag)) { | ||
| return; | ||
| } | ||
|
|
||
| const name = componentName(Component); | ||
| const innerRender = proto.render; | ||
|
|
||
| proto.render = function validatedRender(...renderArgs) { | ||
| warnOutOfRange(name, propName, this.props[propName], allowed); | ||
| return innerRender.apply(this, renderArgs); | ||
| }; | ||
| proto[flag] = true; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the existing validator when bsSizes adds values.
Line 50 retains the first allowed array in the patched render. A later bsSizes call rebuilds values and updates Component.propTypes, but it cannot update that captured array. A valid size from the later decorator then emits an invalid-value warning for class components.
Store mutable validation configuration under flag, and update its allowed value before returning. Add a class-component test with two bsSizes decorators.
Proposed fix
if (Object.prototype.hasOwnProperty.call(proto, flag)) {
+ proto[flag].allowed = allowed;
return;
}
const name = componentName(Component);
const innerRender = proto.render;
proto.render = function validatedRender(...renderArgs) {
- warnOutOfRange(name, propName, this.props[propName], allowed);
+ const validation = proto[flag];
+ warnOutOfRange(
+ validation.name,
+ propName,
+ this.props[propName],
+ validation.allowed
+ );
return innerRender.apply(this, renderArgs);
};
- proto[flag] = true;
+ proto[flag] = { name, allowed };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (Object.prototype.hasOwnProperty.call(proto, flag)) { | |
| return; | |
| } | |
| const name = componentName(Component); | |
| const innerRender = proto.render; | |
| proto.render = function validatedRender(...renderArgs) { | |
| warnOutOfRange(name, propName, this.props[propName], allowed); | |
| return innerRender.apply(this, renderArgs); | |
| }; | |
| proto[flag] = true; | |
| if (Object.prototype.hasOwnProperty.call(proto, flag)) { | |
| proto[flag].allowed = allowed; | |
| return; | |
| } | |
| const name = componentName(Component); | |
| const innerRender = proto.render; | |
| proto.render = function validatedRender(...renderArgs) { | |
| const validation = proto[flag]; | |
| warnOutOfRange( | |
| validation.name, | |
| propName, | |
| this.props[propName], | |
| validation.allowed | |
| ); | |
| return innerRender.apply(this, renderArgs); | |
| }; | |
| proto[flag] = { name, allowed }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/bootstrapUtils.js` around lines 49 - 60, Update the validator setup
around validatedRender so its mutable configuration is stored under flag rather
than captured permanently in the first allowed array. When the existing flag is
found, refresh that configuration’s allowed value before returning, so later
bsSizes calls validate against rebuilt values; add coverage using two bsSizes
decorators on a class component.
| if (typeof Component === 'function' && !isClassComponent) { | ||
| return wrapFunctionComponent(Component, { | ||
| propName, | ||
| defaultValue, | ||
| allowed | ||
| }); | ||
| } | ||
|
|
||
| if (defaultValue !== undefined) { | ||
| Component.defaultProps = { | ||
| ...Component.defaultProps, | ||
| [propName]: defaultValue | ||
| }; | ||
| } | ||
|
|
||
| if (allowed && isClassComponent) { | ||
| patchRenderValidation(Component, propName, allowed); | ||
| } | ||
|
|
||
| return Component; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find React exotic components and Bootstrap decorator applications.
rg -n -C 4 --glob '*.js' '\b(?:React\.)?(?:memo|forwardRef)\s*\(' src test
rg -n -C 4 --glob '*.js' '\bbs(?:Class|Styles|Sizes)\s*\(' src testRepository: mattermost/react-bootstrap
Length of output: 24887
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- bootstrapUtils structure ---'
ast-grep outline src/utils/bootstrapUtils.js
printf '%s\n' '--- bootstrapUtils implementation ---'
cat -n src/utils/bootstrapUtils.js | sed -n '1,190p'
printf '%s\n' '--- bootstrapUtils tests ---'
cat -n test/utils/bootstrapUtilsSpec.js | sed -n '1,290p'
printf '%s\n' '--- React and test configuration ---'
rg -n -C 3 '"react"|"react-dom"|"react-test-renderer"|React 19|reactVersion|peerDependencies' package.json yarn.lock package-lock.json npm-shrinkwrap.json . 2>/dev/null | head -n 240Repository: mattermost/react-bootstrap
Length of output: 27981
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- decorator call-path probe ---'
node - <<'JS'
function curry(fn) {
return (...args) => {
const last = args[args.length - 1];
if (typeof last === 'function') return fn(...args);
return Component => fn(...args, Component);
};
}
const applyCalls = [];
const decorate = curry((name, Component) => {
applyCalls.push({ name, Component });
return Component;
});
const exoticTypes = {
memo: { $$typeof: Symbol.for('react.memo'), type: () => null },
forwardRef: { $$typeof: Symbol.for('react.forward_ref'), render: () => null }
};
for (const [kind, component] of Object.entries(exoticTypes)) {
const result = decorate('bsStyle', component);
console.log(JSON.stringify({
kind,
inputType: typeof component,
returnedType: typeof result,
returnedIsDecorator: typeof result === 'function',
applyCalled: applyCalls.some(call => call.Component === component)
}));
}
JS
printf '%s\n' '--- remaining bsSizes implementation ---'
cat -n src/utils/bootstrapUtils.js | sed -n '171,214p'
printf '%s\n' '--- all direct decorator call forms involving exotic components ---'
rg -n -C 5 --glob '*.js' '(memo|forwardRef)|bsClass|bsStyles|bsSizes' src test | rg -n -C 2 '(memo|forwardRef)|bsClass|bsStyles|bsSizes' | head -n 260Repository: mattermost/react-bootstrap
Length of output: 12593
🌐 Web query:
React 19 defaultProps function components memo forwardRef element type official upgrade guide
💡 Result:
In React 19, support for defaultProps on function components has been removed [1][2]. The official upgrade path is to replace the use of defaultProps with standard ES6 default parameters in your function's destructuring parameter list [1][3]. Key details regarding this change: Function Components: You must use native JavaScript default parameters [1][3]. For example, instead of defining Component.defaultProps = { prop: value }, you should define the component as function Component({ prop = value }) {... } [1]. Class Components: Class components continue to support static defaultProps, as there is no equivalent ES6 pattern for them [1][2]. Higher-Order Components (memo, forwardRef): When using React.memo or forwardRef with a function component, the default values should be placed in the signature of the inner component function [3]. If you are using TypeScript, this change also simplifies type definitions, as you can rely on standard optional properties and default parameter inference [4][5]. The React team has provided codemods to automate this migration, such as the react/19/replace-default-props codemod available in the official codemod registry [6]. Note: propTypes have also been removed from React 19 and are now silently ignored; if runtime type checking is required, it is recommended to migrate to a type-checking solution like TypeScript [1][2].
Citations:
- 1: https://react.dev/blog/2024/04/25/react-19-upgrade-guide
- 2: https://github.com/facebook/react/releases/tag/v19.0.0
- 3: https://errornotes.dev/en/errors/react/fixing-the-defaultprops-deprecation-warning-in-react-183-and-19
- 4: https://github.com/typescript-cheatsheets/react/blob/main/docs/basic/getting-started/default-props.md
- 5: https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/default_props/
- 6: https://app.codemod.com/registry/react-19-replace-default-props
Handle React exotic component types.
curry treats React.memo() and React.forwardRef() results as objects, so it returns a decorator without applying bsClass, bsStyles, or bsSizes. Extend component detection, then preserve refs and memoization while applying defaults and validation. Add React 19 tests for all three decorators on both component types.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/bootstrapUtils.js` around lines 99 - 118, Update curry to detect
React.memo and React.forwardRef results as supported component types instead of
treating them as plain objects. Apply bsClass, bsStyles, bsSizes, default props,
and validation while preserving each component’s memoization or ref behavior.
Add React 19 coverage for all three decorators on both exotic component types.
1 participant
Summary
React 19 makes two changes to those:
propTypesis no longer checked at runtime, so some tests that relied on the warnings printed by that have been replaced with regular runtime checks usingwarningwhich only function when theNODE_ENVisn't production. Some custom validators had to be rewritten for this.defaultProps.The source code for this adds a lot of those by using some helpers like
bsSizewhich had to be rewritten to adapt to that.Ticket Link
https://mattermost.atlassian.net/browse/MM-69835