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

Change type of graphicalItems from any to ReadonlyArray<ReactElement> #3844

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

PavelVanecek
Copy link
Collaborator

Description

Types!

Related Issue

#3717

Motivation and Context

Types

How Has This Been Tested?

TS, Jest

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 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.
  • I have added a storybook story or extended an existing story to show my changes
  • All new and existing tests passed.

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
Seldaek Jordi Boggiano
Copy link
Member

@ckifer ckifer left a comment

Choose a reason for hiding this comment

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

Looks good, just one question on the class thing

@@ -146,7 +146,7 @@ export class Bar extends PureComponent<Props, State> {
offset,
}: {
props: Props;
item: Bar;
Copy link
Member

Choose a reason for hiding this comment

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

was Bar referencing the class in this case? :o. Is that not still more accurate than a generic ReactElement?

Copy link
Member

Choose a reason for hiding this comment

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

Could we do ReactElement<BarProps> to give the needed specificity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So two answers, one short term (this PR) one long term (recharts future).

Short term: Yes, using Bar is more accurate. But it is only accurate on the receiving end! In the sending end there is no guarantee. generateCategoricalChart sends ReactElement. So this was a false positive, going against Barbara Liskov principle.

Long term: In my opinion, architecture where a component requires a rendered instance of itself to look up in the global state before it decides if it can render - is not a good architecture. This whole function should be different and this argument should not exist.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed with the long term for sure, one of the major issues with why generateCategoricalChart is how it is.

Unfortunate that short term we can't have some sort of guarantee of what we're receiving, but I guess that's just the cards we've been deal with this little mess.

Thanks

@@ -874,6 +874,7 @@ export const generateCategoricalChart = ({
(axisMap && axisMap[id]) || entry.axisType === 'zAxis',
`Specifying a(n) ${entry.axisType}Id requires a corresponding ${
entry.axisType
// @ts-expect-error we should stop reading data from ReactElements
Copy link
Member

Choose a reason for hiding this comment

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

💯% agree, the data has to come from somewhere though. Eventually, we refactor this to get the data from... well data instead of elements 😅. In the meantime the invariant is still useful with the expect error

@ckifer ckifer merged commit ffb6b4d into recharts:master Oct 12, 2023
@PavelVanecek PavelVanecek deleted the graphicalItems-types branch October 13, 2023 02:36
GMer78 pushed a commit to GMer78/recharts-1 that referenced this pull request Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants