fix: guard against non-function d3-scale exports in getD3ScaleFromType#7123
Merged
PavelVanecek merged 6 commits intoMar 22, 2026
Merged
Conversation
tdebarochez
changed the title
tdebarochez
marked this pull request as ready for review
March 10, 2026 13:59
Contributor
WalkthroughReplaces direct property access on d3Scales with a local Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
Collaborator
|
We're going to need a test for this. How to reproduce? |
Contributor
Author
|
@PavelVanecek I found the issue while switching from one branch to another on a big project and regenerating my bun.lock file. |
Collaborator
|
If we won't test then it's likely to come back later. |
Contributor
Author
|
Of course, I completely understand. |
Collaborator
|
Better like this. If you prefer you can also add a full app with bun and all and put it in https://github.com/recharts/recharts-integ/tree/main/integrations. We don't have anything for bun yet. |
PavelVanecek
approved these changes
Mar 11, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7123 +/- ##
==========================================
+ Coverage 89.61% 89.62% +0.01%
==========================================
Files 536 536
Lines 40479 40480 +1
Branches 5519 5521 +2
==========================================
+ Hits 36275 36282 +7
+ Misses 4196 4190 -6
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will decrease total bundle size by 166 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-treeshaking-treemapAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-sankeyAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-polarAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-sunburstAssets Changed:
|
Ensures d3Scales entries are functions before invoking them, preventing runtime "not a function" errors when a scale name resolves to a non-callable export.
tdebarochez
force-pushed
the
fix/d3Scales-name-not-a-function
branch
from
March 11, 2026 12:57
64aa020 to
5de819e
Compare
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/state/selectors/combiners/combineConfiguredScale.ts (2)
17-24: Runtime guards correctly prevent TypeError on non-function exports.The
typeof === 'function'guards effectively fix the issue wherescaleImplicit(a Symbol) would cause a runtime error when invoked. The use ofunknownoveranyis the right choice.Note on
asassertions: The coding guidelines prohibitastype assertions except foras const. Lines 17, 19, and 23 useasassertions. While TypeScript cannot automatically narrow indexed access types after atypeofcheck, you could consider a type guard pattern to eliminate the assertions:♻️ Optional refactor using a type guard
+type ScaleFactory<Domain extends CategoricalDomainItem> = () => CustomScaleDefinition<Domain>; + +function isScaleFactory<Domain extends CategoricalDomainItem>( + value: unknown, +): value is ScaleFactory<Domain> { + return typeof value === 'function'; +} + function getD3ScaleFromType<Domain extends CategoricalDomainItem = CategoricalDomainItem>( realScaleType: D3ScaleType | RechartsScaleType, ): CustomScaleDefinition<Domain> | undefined { - const scales = d3Scales as Record<string, unknown>; - if (realScaleType in scales && typeof scales[realScaleType] === 'function') { - return (scales[realScaleType] as () => CustomScaleDefinition<Domain>)(); + const scales: Record<string, unknown> = d3Scales; + const directScale = scales[realScaleType]; + if (isScaleFactory<Domain>(directScale)) { + return directScale(); } const name = `scale${upperFirst(realScaleType)}`; - if (name in scales && typeof scales[name] === 'function') { - return (scales[name] as () => CustomScaleDefinition<Domain>)(); + const namedScale = scales[name]; + if (isScaleFactory<Domain>(namedScale)) { + return namedScale(); } return undefined; }Given the runtime safety is ensured by the
typeofchecks and this is a pragmatic solution, this can be deferred if needed. As per coding guidelines: "Do not useastype assertions. The only exception isas const".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/selectors/combiners/combineConfiguredScale.ts` around lines 17 - 24, The code uses forbidden `as` assertions on lines that index into `scales`; replace those casts by adding a type guard (e.g. isScaleFactory(value): value is () => CustomScaleDefinition<Domain>) and use it for the checks around `scales[realScaleType]` and `scales[name]` so you can safely call the returned function without `as` assertions; reference `d3Scales`, `scales`, `realScaleType`, `name`, `upperFirst`, and `CustomScaleDefinition` when implementing the guard and remove the `as` usages.
14-26: The inconsistency concern is valid, but the specific example is misleading.The code in
combineConfiguredScale.tsvalidates that the value is both ind3Scalesand is a function, whileisSupportedScaleNameonly checks theinoperator. However,isSupportedScaleName('implicit')would not actually be a problem since'implicit'is not part of theRechartsScaleTypeunion type—all validRechartsScaleTypevalues have corresponding function exports ind3-scale.That said, updating
isSupportedScaleNameto also verify callability would improve consistency and defensive programming:function isSupportedScaleName(name: string): name is RechartsScaleType { const scaleName = getD3ScaleName(name); return scaleName in d3Scales && typeof d3Scales[scaleName as keyof typeof d3Scales] === 'function'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/selectors/combiners/combineConfiguredScale.ts` around lines 14 - 26, Update the isSupportedScaleName type guard to mirror the defensive check used in getD3ScaleFromType: compute the canonical name via getD3ScaleName(name), then verify that this key exists in d3Scales and that typeof d3Scales[scaleName] === 'function' before returning true; ensure the function signature remains a type predicate (name is RechartsScaleType) so callers relying on the guard (and getD3ScaleFromType) are safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/state/selectors/combiners/combineConfiguredScale.ts`:
- Around line 17-24: The code uses forbidden `as` assertions on lines that index
into `scales`; replace those casts by adding a type guard (e.g.
isScaleFactory(value): value is () => CustomScaleDefinition<Domain>) and use it
for the checks around `scales[realScaleType]` and `scales[name]` so you can
safely call the returned function without `as` assertions; reference `d3Scales`,
`scales`, `realScaleType`, `name`, `upperFirst`, and `CustomScaleDefinition`
when implementing the guard and remove the `as` usages.
- Around line 14-26: Update the isSupportedScaleName type guard to mirror the
defensive check used in getD3ScaleFromType: compute the canonical name via
getD3ScaleName(name), then verify that this key exists in d3Scales and that
typeof d3Scales[scaleName] === 'function' before returning true; ensure the
function signature remains a type predicate (name is RechartsScaleType) so
callers relying on the guard (and getD3ScaleFromType) are safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d51aeb76-b6c0-4521-a255-fe034157c37e
📒 Files selected for processing (2)
src/state/selectors/combiners/combineConfiguredScale.tstest/state/selectors/combiners/combineConfiguredScale.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/state/selectors/combiners/combineConfiguredScale.spec.ts
| // @ts-expect-error we should do better type verification here | ||
| return d3Scales[name](); | ||
| if (name in scales && typeof scales[name] === 'function') { | ||
| return (scales[name] as () => CustomScaleDefinition<Domain>)(); |
Collaborator
There was a problem hiding this comment.
Btw the "as" has the exact same type safety as the comment (= none whatsoever) but it pretends that it does. I would prefer the comment. At least that way it's obvious that this has no type safety.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/state/selectors/combiners/combineConfiguredScale.ts (1)
17-23:⚠️ Potential issue | 🟠 MajorUse own-key lookup and remove the forbidden
asassertion.Line 17 uses an
asassertion (disallowed by repo rules), and Lines 18/22 usein, which can match inherited function keys. Prefer own-property resolution plus runtime narrowing.♻️ Proposed fix
function getD3ScaleFromType<Domain extends CategoricalDomainItem = CategoricalDomainItem>( realScaleType: D3ScaleType | RechartsScaleType, ): CustomScaleDefinition<Domain> | undefined { - const scales = d3Scales as Record<string, unknown>; - if (realScaleType in scales && typeof scales[realScaleType] === 'function') { - return scales[realScaleType](); - } + const resolveScaleFactory = (key: PropertyKey): (() => CustomScaleDefinition<Domain>) | undefined => { + if (!Object.prototype.hasOwnProperty.call(d3Scales, key)) { + return undefined; + } + const candidate = Reflect.get(d3Scales, key); + return typeof candidate === 'function' ? candidate : undefined; + }; + + const directFactory = resolveScaleFactory(realScaleType); + if (directFactory != null) { + return directFactory(); + } + const name = `scale${upperFirst(realScaleType)}`; - if (name in scales && typeof scales[name] === 'function') { - return scales[name](); + const prefixedFactory = resolveScaleFactory(name); + if (prefixedFactory != null) { + return prefixedFactory(); } return undefined; }#!/bin/bash # Verify this file no longer contains the forbidden cast or `in scales` pattern. rg -n "d3Scales\s+as\s+Record|in\s+scales" src/state/selectors/combiners/combineConfiguredScale.tsAs per coding guidelines: "
src/**/*.{ts,tsx}: Do not useastype assertions. The only exception isas const."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/selectors/combiners/combineConfiguredScale.ts` around lines 17 - 23, Remove the forbidden "as" type assertion and stop using the `in scales` inherited-key check; instead use the original d3Scales object directly and perform own-key lookups with Object.prototype.hasOwnProperty.call(d3Scales, key) (where key is realScaleType or `scale${upperFirst(realScaleType)}`), then runtime-narrow the retrieved value with typeof === 'function' and call it (e.g., const candidate = d3Scales[key]; if (Object.prototype.hasOwnProperty.call(d3Scales, key) && typeof candidate === 'function') return candidate();). Ensure you update the lookup logic around the symbols d3Scales, realScaleType, upperFirst and the scale factory calls to use this pattern and remove the `as` cast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/state/selectors/combiners/combineConfiguredScale.ts`:
- Around line 17-23: Remove the forbidden "as" type assertion and stop using the
`in scales` inherited-key check; instead use the original d3Scales object
directly and perform own-key lookups with
Object.prototype.hasOwnProperty.call(d3Scales, key) (where key is realScaleType
or `scale${upperFirst(realScaleType)}`), then runtime-narrow the retrieved value
with typeof === 'function' and call it (e.g., const candidate = d3Scales[key];
if (Object.prototype.hasOwnProperty.call(d3Scales, key) && typeof candidate ===
'function') return candidate();). Ensure you update the lookup logic around the
symbols d3Scales, realScaleType, upperFirst and the scale factory calls to use
this pattern and remove the `as` cast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44f4f30a-737e-4975-8451-0c9bc061b5e1
📒 Files selected for processing (2)
src/state/selectors/combiners/combineConfiguredScale.tstest/state/selectors/combiners/combineConfiguredScale.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/state/selectors/combiners/combineConfiguredScale.spec.ts
Collaborator
|
@tdebarochez may I please ask you to rebase or merge main? I fixed the builds in another PR. |
PavelVanecek
approved these changes
Mar 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add
typeof === 'function'guards ingetD3ScaleFromType(andisSupportedScaleNamein v3.7+) before calling d3-scale exports. Theinoperator returnstrueforscaleImplicit, which is aSymbol(not a function), causing aTypeError: d3Scales[name] is not a functionat runtime.Related Issue
N/A — discovered in production usage with
victory-vendor/d3-scalere-exports.Motivation and Context
d3-scaleexportsscaleImplicitasSymbol("implicit")(viad3-scale/src/ordinal.js). When recharts imports* as d3Scalesfromvictory-vendor/d3-scale(which re-exports everything fromd3-scale), the namespace object includesscaleImplicit.In
getD3ScaleFromType, the checkname in d3Scalespasses for"scaleImplicit", butd3Scales["scaleImplicit"]()throws because a Symbol is not callable.This surfaces depending on how the bundler resolves the
victory-vendorESM re-exports — some bundlers tree-shakescaleImplicitout of the namespace, others preserve it.How Has This Been Tested?
scaleImplicitis correctly skipped while all validscale*functions continue to workTypes of changes
Checklist
Summary by CodeRabbit
Refactor
Tests
No user-facing changes.