Feat/es toolkit inlining by daiboom · Pull Request #6543 · recharts/recharts

daiboom · 2025-10-31T06:07:46Z

Description

Related Issue

Inlined noop and isNotNil from es-toolkit into src/util/DataUtils.ts and updated all call sites to use these helpers. Added/updated unit tests accordingly

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
I have added a storybook story or VR test, or extended an existing story or VR test to show my changes

Summary by CodeRabbit

Chores
- Internal utility functions consolidated and reorganized to improve code maintainability and reduce external dependencies. No visible changes to product functionality or user experience.

…Utils

…implementation in DataUtils (storybook)

coderabbitai · 2025-10-31T06:08:00Z

Walkthrough

The changes consolidate utility functions by centralizing noop and isNotNil implementations in src/util/DataUtils.ts and updating imports across the codebase to use these centralized versions instead of importing from es-toolkit or defining locally, creating a single source of truth for commonly used utilities.

Changes

Cohort / File(s)	Change Summary
Centralized utility exports `src/util/DataUtils.ts`	Added public exports for `isNotNil<T>` (type-guard for nullish check) and `noop` (no-op function); minor documentation spacing adjustment to `upperFirst`
Import noop from DataUtils `src/animation/CSSTransitionAnimate.tsx`, `src/animation/JavascriptAnimate.tsx`, `src/component/ResponsiveContainer.tsx`, `src/zindex/ZIndexLayer.tsx`, `storybook/storybook-addon-recharts/RechartsInspectorDecorator.tsx`	Updated import source for `noop` from `es-toolkit` to `../util/DataUtils` (or relative equivalent)
Import isNotNil from DataUtils `src/cartesian/Brush.tsx`, `src/state/selectors/axisSelectors.ts`	Updated import source for `isNotNil` from `es-toolkit` to `../util/DataUtils`; consolidated with other DataUtils imports where applicable
Replace local noop with import `src/state/SetLegendPayload.ts`, `src/synchronisation/useChartSynchronisation.tsx`	Removed local `noop` constant/function definition and replaced with import from `../util/DataUtils`
Test coverage `test/util/DataUtils.spec.ts`	Added test cases for `isNotNil` (nullish checks and type narrowing) and `noop` (void return and callback usage)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Specific areas to review:
- Verify import paths are correctly adjusted relative to each file's location
- Confirm isNotNil and noop implementations in DataUtils.ts match the semantics of previous es-toolkit imports and local definitions
- Validate test coverage adequately demonstrates type-guard behavior of isNotNil and no-op semantics

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The PR description is largely incomplete against the provided template. While the Related Issue (#6530) is correctly referenced and the "Bug fix" type is selected with tests marked as added, the critical sections are unfilled: the Description field contains only template comments, the Motivation and Context section is empty, and the How Has This Been Tested section provides no details about what testing was performed. The template requires meaningful content in these sections to explain what was changed, why it was necessary, and how it was verified.	The author should provide a complete PR description by filling in the Description section with details of the inlining work (which utilities were moved and to where), explaining the Motivation and Context (why this change was necessary, likely referencing issue #6530), and detailing How Has This Been Tested (which test files were added or updated, what scenarios were covered, and any manual testing performed). These details are essential for code review and repository history.
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title "Feat/es toolkit inlining" directly describes the primary change in the changeset: inlining the `noop` and `isNotNil` utilities from the es-toolkit package into the local codebase (`src/util/DataUtils.ts`) and updating all call sites accordingly. The title is concise, clear, and specific enough that a teammate reviewing the commit history would understand the main objective of this PR without scanning the full changeset details.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d50cbea and c547059.

📒 Files selected for processing (11)

src/animation/CSSTransitionAnimate.tsx (1 hunks)
src/animation/JavascriptAnimate.tsx (1 hunks)
src/cartesian/Brush.tsx (1 hunks)
src/component/ResponsiveContainer.tsx (1 hunks)
src/state/SetLegendPayload.ts (1 hunks)
src/state/selectors/axisSelectors.ts (1 hunks)
src/synchronisation/useChartSynchronisation.tsx (1 hunks)
src/util/DataUtils.ts (2 hunks)
src/zindex/ZIndexLayer.tsx (1 hunks)
storybook/storybook-addon-recharts/RechartsInspectorDecorator.tsx (1 hunks)
test/util/DataUtils.spec.ts (2 hunks)

🧰 Additional context used

📓 Path-based instructions (3)

src/**/*.{js,jsx,ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Do not hardcode any user-facing strings or formatting choices in the library code; leave internationalization to library users

Files:

src/synchronisation/useChartSynchronisation.tsx
src/zindex/ZIndexLayer.tsx
src/animation/CSSTransitionAnimate.tsx
src/component/ResponsiveContainer.tsx
src/util/DataUtils.ts
src/state/selectors/axisSelectors.ts
src/state/SetLegendPayload.ts
src/cartesian/Brush.tsx
src/animation/JavascriptAnimate.tsx

storybook/**

📄 CodeRabbit inference engine (DEVELOPING.md)

Use Storybook for high-fidelity examples intended for publication; prefer unit or VR tests for low-fidelity cases due to Chromatic limits

Files:

storybook/storybook-addon-recharts/RechartsInspectorDecorator.tsx

{test,www/test}/**

📄 CodeRabbit inference engine (DEVELOPING.md)

Place unit tests in the test directory; some tests may also live in www/test

Files:

test/util/DataUtils.spec.ts

🧠 Learnings (3)

📚 Learning: 2025-10-25T07:35:46.180Z

Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.180Z
Learning: Applies to test/**/*.test.{ts,tsx} : Mock getBoundingClientRect using test/helper/MockGetBoundingClientRect.ts when rendering Recharts components (e.g., Tooltip, Legend, charts)

Applied to files:

src/component/ResponsiveContainer.tsx
src/cartesian/Brush.tsx
storybook/storybook-addon-recharts/RechartsInspectorDecorator.tsx

📚 Learning: 2025-10-25T07:36:02.211Z

Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-25T07:36:02.211Z
Learning: Recharts aims for simple, declarative, and composable charts; prioritize consistency, usability, performance, and accessibility

Applied to files:

src/cartesian/Brush.tsx
storybook/storybook-addon-recharts/RechartsInspectorDecorator.tsx

📚 Learning: 2025-10-25T07:34:46.549Z

Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-10-25T07:34:46.549Z
Learning: Applies to storybook/** : Use Storybook for high-fidelity examples intended for publication; prefer unit or VR tests for low-fidelity cases due to Chromatic limits

Applied to files:

storybook/storybook-addon-recharts/RechartsInspectorDecorator.tsx

🧬 Code graph analysis (1)

test/util/DataUtils.spec.ts (1)

src/util/DataUtils.ts (2)

isNotNil (201-203)

noop (209-209)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Build, Test, Pack

🔇 Additional comments (12)

src/zindex/ZIndexLayer.tsx (1)

4-4: LGTM! Clean import path update.

The change from es-toolkit to the centralized ../util/DataUtils aligns with the PR objective to inline commonly used utilities and reduce external dependencies.

storybook/storybook-addon-recharts/RechartsInspectorDecorator.tsx (1)

4-4: LGTM! Consistent with codebase-wide refactor.

The import path update centralizes the noop utility, maintaining consistency with other files in this PR.

src/component/ResponsiveContainer.tsx (1)

17-17: LGTM! Consolidates utility imports.

Both isNumber and noop are now sourced from the centralized DataUtils module, reducing dependency on es-toolkit while maintaining the same functionality.

src/animation/JavascriptAnimate.tsx (1)

3-3: LGTM! Standard import path update.

The change aligns with the broader refactoring effort to centralize utility functions.

src/state/selectors/axisSelectors.ts (1)

46-55: LGTM! Import consolidation for type guard utility.

The addition of isNotNil to the DataUtils imports centralizes this type guard utility, which is used effectively at line 555 to filter number arrays. The change maintains type safety while reducing external dependencies.

src/state/SetLegendPayload.ts (1)

7-7: LGTM! Eliminates local duplication.

Replacing the local noop definition with a centralized import promotes code reuse and maintains consistency across the codebase.

src/animation/CSSTransitionAnimate.tsx (1)

3-3: LGTM! Consistent import update.

The change maintains consistency with the parallel update in JavascriptAnimate.tsx and aligns with the overall refactoring strategy.

src/synchronisation/useChartSynchronisation.tsx (1)

17-17: LGTM! Centralizes utility function.

Replacing the local noop definition with the centralized import from DataUtils reduces code duplication and maintains consistency with the rest of the codebase.

src/util/DataUtils.ts (2)

196-203: LGTM! Clean implementation of isNotNil type guard.

The use of != (loose equality) correctly checks for both null and undefined in a single comparison, and the NonNullable<T> type guard properly narrows the type for downstream usage.

205-209: LGTM! Standard no-op implementation.

Simple and correct placeholder function that can be used as a default callback.

src/cartesian/Brush.tsx (1)

19-19: LGTM! Clean import consolidation.

Consolidating isNotNil and isNumber imports from DataUtils improves maintainability by reducing external dependencies.

test/util/DataUtils.spec.ts (1)

288-327: LGTM! Comprehensive test coverage for the new utilities.

The tests thoroughly validate:

isNotNil: null/undefined handling, falsy value handling, and type refinement via .filter()

noop: return value, error-free execution, and usage as a callback placeholder

The type refinement tests are particularly valuable for confirming the type guard behavior.

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

ckifer

Lgtm

codecov · 2025-10-31T06:17:47Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.62%. Comparing base (d50cbea) to head (c547059).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6543   +/-   ##
=======================================
  Coverage   91.62%   91.62%           
=======================================
  Files         483      483           
  Lines       40266    40267    +1     
  Branches     4568     4568           
=======================================
+ Hits        36895    36896    +1     
  Misses       3355     3355           
  Partials       16       16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

PavelVanecek

It would be great to have an eslint rule to prevent this from happening again.

daiboom · 2025-10-31T09:43:22Z

It would be great to have an eslint rule to prevent this from happening again.
@PavelVanecek @ckifer
Sounds good. I’ll look into adding an ESLint rule to catch unused or incorrect external imports and can draft one if you’re ok with that.

daiboom · 2025-10-31T10:09:04Z

add a custom ESLint rule in this PR?

PavelVanecek · 2025-10-31T10:40:58Z

Here or next PR, either is fine.

daiboom · 2025-10-31T11:19:31Z

@PavelVanecek open a PR soon.

daiboom added 4 commits October 31, 2025 14:53

feat: add util function isNotNil, noop

83a149c

chore: replace es-toolkit noop with inline implementation in DataUtils

8a5b611

chore: replace es-toolkit isNotNil with inline implementation in Data…

228b545

…Utils

chore: replace noop function created in replace function with inline …

c547059

…implementation in DataUtils (storybook)

ckifer approved these changes Oct 31, 2025

View reviewed changes

PavelVanecek approved these changes Oct 31, 2025

View reviewed changes

ckifer merged commit 770206f into recharts:main Oct 31, 2025
22 checks passed

coderabbitai bot mentioned this pull request Feb 8, 2026

feat: accept data-* attributes (including data-testid) on components #6976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feat/es toolkit inlining#6543

Feat/es toolkit inlining#6543
ckifer merged 4 commits intorecharts:mainfrom
daiboom:feat/es-toolkit-inlining

daiboom commented Oct 31, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 31, 2025 •

edited

Loading

Uh oh!

ckifer left a comment

Uh oh!

codecov bot commented Oct 31, 2025 •

edited

Loading

Uh oh!

PavelVanecek left a comment

Uh oh!

daiboom commented Oct 31, 2025 •

edited

Loading

Uh oh!

daiboom commented Oct 31, 2025

Uh oh!

PavelVanecek commented Oct 31, 2025

Uh oh!

daiboom commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

daiboom commented Oct 31, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

ckifer left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PavelVanecek left a comment

Choose a reason for hiding this comment

Uh oh!

daiboom commented Oct 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daiboom commented Oct 31, 2025

Uh oh!

PavelVanecek commented Oct 31, 2025

Uh oh!

daiboom commented Oct 31, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daiboom commented Oct 31, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 31, 2025 •

edited

Loading

codecov bot commented Oct 31, 2025 •

edited

Loading

daiboom commented Oct 31, 2025 •

edited

Loading