Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enables strict null checks across the recharts codebase, addressing approximately 900 TypeScript errors primarily in tests and storybook configurations. The changes focus on making the codebase compatible with TypeScript's strictNullChecks compiler option.
Key changes:
- Added
strictNullChecks: trueto test and storybook TypeScript configurations - Updated type annotations to explicitly handle
nullandundefinedvalues - Added runtime null checks and assertions where needed
- Fixed incorrect type expectations and fallback values
Reviewed Changes
Copilot reviewed 117 out of 118 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/tsconfig.json | Enabled strictNullChecks compiler option |
| storybook/tsconfig.json | Enabled strictNullChecks compiler option |
| test/**/*.spec.tsx | Added null checks, type guards, and updated type annotations for test expectations |
| storybook/**/*.tsx | Added null guards and updated type signatures for storybook components |
| src/util/types.ts | Updated return types to allow ReactNode instead of restrictive JSX.Element |
| src/component/Legend.tsx | Documented itemSorter prop to clarify null behavior |
| www/src/docs/**/*.tsx | Fixed null handling in example components |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -94,8 +94,8 @@ const HighlightAndZoomLineChart = () => { | |||
| ...prev, | |||
| refAreaLeft: undefined, | |||
| refAreaRight: undefined, | |||
There was a problem hiding this comment.
[nitpick] The fallback logic uses initialState.left and initialState.right which are defined as 'dataMin' and 'dataMax' respectively. This is semantically correct, but the original code's intent ('dataMin'/'dataMax') is now obscured. Consider adding a comment explaining why these specific fallback values are used, or using the string literals directly if the initialState values could change.
| refAreaRight: undefined, | |
| refAreaRight: undefined, | |
| // Fallback to initialState.left ('dataMin') and initialState.right ('dataMax') to reset zoom window to full data range. |
| const tooltipSettings1: TooltipPayloadConfiguration = { | ||
| positions: undefined, | ||
| settings: undefined, | ||
| settings: { nameKey: 'y' }, |
There was a problem hiding this comment.
[nitpick] The test previously used undefined for settings but now requires { nameKey: 'y' }. While this fixes the strict null check issue, verify that using 'y' as the nameKey is semantically correct for this test case and doesn't inadvertently change what behavior is being tested.
| settings: { nameKey: 'y' }, | |
| settings: undefined, |
|
|
||
| describe('LineChart - test ref access', () => { | ||
| test('should allow access to the main SVG through the ref prop forwarded from CategoricalChart', () => { | ||
| expect.assertions(2); |
There was a problem hiding this comment.
[nitpick] The addition of expect.assertions(2) is good practice to ensure the ref callback is actually invoked. However, the test logic was restructured to move assertions inside the ref callback. If the ref callback is never called, the test would fail with 'Expected 2 assertions, but received 0' rather than a more descriptive error. Consider if this is the desired failure mode.
There was a problem hiding this comment.
Yeah that's the point.
| refAreaLeft: undefined, | ||
| refAreaRight: undefined, |
There was a problem hiding this comment.
The initial state was changed from empty strings ('') to undefined. While this is more type-safe, verify that the logic in getAxisYDomain and the zoom handler correctly handles undefined vs empty string, as these may have been treated differently in conditional checks.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6497 +/- ##
==========================================
- Coverage 93.45% 93.45% -0.01%
==========================================
Files 431 431
Lines 39169 39168 -1
Branches 4552 4550 -2
==========================================
- Hits 36607 36606 -1
Misses 2545 2545
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 14 bytes (0.0%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
|
3a5cf4c to
750d046
Compare
ckifer
left a comment
There was a problem hiding this comment.
Nice 🚀🚀🚀
No more accidentally adding new ones now
900 or so errors, mostly in tests and storybook -> 0
Closes #5768