Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: filter out functions in filterProps, remove type from valid SVGElementPropKeys #3327

Merged
merged 6 commits into from
Feb 6, 2023

Conversation

ckifer
Copy link
Member

@ckifer ckifer commented Feb 3, 2023

Description

Recharts has a filterProps function which was intended to filter out all properties that are not valid SVGProps or events that need passed down to children.

Multiple times we have come across issues where Recharts defined props such as type (in the past points) overlap with traditional SVGProps (https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute)

In these cases props get spread onto SVG elements that aren't intended or valid.

In this case the type prop is a valid SVG attribute. This never caused error in the past because the DOM will ignore attributes that are strings or numbers, but when adding a line or area type property that is custom CurveFactory from d3 such as

const stepAround = curveCardinal.tension(0.5);

the DOM will throw an error "Invalid value for prop type on tag" because functions are not valid on HTML elements in the DOM.

  • remove type from valid SVGElementAttributes - we do not use any elements that need it
  • add an isFunction check within filterProps for attributes that are supposed to be one of SVGElementAttributes - these should never be functions
  • detail comment within filterProps
  • adds an example using a CurveFactory in AreaChart demo

Related Issue

#3310

Motivation and Context

Resolve above described error. Prevent more errors with type in the future.

How Has This Been Tested?

The true type SVG attribute can only be used with the following tags:

image

None of which we use in Recharts.

  • In demo slightly modify an example to use a CurveFactory - ensure the new curve factory type is displayed correctly and that the error is not thrown.
    • type is passed down explicitly by the chart so it is okay that it gets filtered out in filterProps
  • check functionality of other chart types and CurveType values such as monotone and linear

Screenshots (if appropriate):

image

^ No error thrown like in issue but curve still reflects the given CurveFactory

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 code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@ckifer ckifer added the bug General bug label label Feb 3, 2023
Comment on lines 317 to 318
(!_.isFunction(inputProps?.[key]) &&
((svgElementType && matchingElementTypeKeys.includes(key)) || SVGElementPropKeys.includes(key))) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the comment. How about refactoring this into a separate function? Such a separate function could then be unit tested nicely.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, the point of the function is to filter props and this is the part that does the filtration. (imo) sometimes factoring out too much can lead to more confusion, but in this case something like isValidSpreadableProp maybe would do for the check itself?

@nikolasrieble nikolasrieble self-requested a review February 5, 2023 10:50
Copy link
Contributor

@nikolasrieble nikolasrieble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good cleanup, thank you! It would be nice to capture in a comment as to why TYPE in no longer in the list of SVGElementPropKeys.

Either way this already is an improvement.

/**
* Props are blindly spread onto SVG elements. This loop filters out properties that we don't want to spread.
* Items filtered out are as follows:
* - functions in properties that are SVG attributes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* - functions in properties that are SVG attributes
* - functions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All functions are filtered out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if includeEvents is true then there are functions that are passed down. includeEvents is only set to true when props are spread via our components (such as Curve) and never when props are spread onto an SVG element

Copy link
Contributor

@nikolasrieble nikolasrieble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see any issue and could not find any while testing.
Please do consider my refactoring suggestion.

Good to go.

@ckifer ckifer reopened this Feb 6, 2023
@ckifer
Copy link
Member Author

ckifer commented Feb 6, 2023

Should have requested updates now!

@ckifer ckifer merged commit d8ea0c8 into recharts:master Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug General bug label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants