refactor default props to fix error message

[andre19980]

This PR is related to issue #2415.

Refactor default props declared like:

Component.defaultProps = {
    prop1: 0,
    prop2: 1,
    // ....
}

Now, the default props are defined directly inside the component

export const Component = props => {
    const {
      prop1 = 0,
      prop2 = 1,
      // ...
    } = props
    // ...
}

The given solution is based in the issue from another repository:

• FunctionComp.defaultProps is going to be deprecated in React 18.3.0 chakra-ui/chakra-ui#7057

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nivo	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 1, 2023 2:41am

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 93690b0:

Sandbox	Source
nivo	Configuration

[plouc]

@andre19980, could you please rebase this PR?

[andre19980]

@plouc it worked! Thanks :)

[plouc]

@andre19980, it seems like the tooltip doesn't work anymore for the choropleth (SVG implementation), see https://nivo.rocks/choropleth/ VS https://nivo-git-fork-andre19980-refactor-default-props-of-b2d03b-plouc.vercel.app/choropleth/.

[plouc]

A test is also failing for the line chart.

[ofps]

I hope you guys don't mind, but I saw this PR and decided to take a look.

From my investigation it looks like the problem happens when we explicitly pass undefined as a prop.

The failing tests from the line chart explicitly define axisLeft and axisBottom as undefined. If we comment those lines all tests pass but this actually reveals a bigger issue.

When we do something like {...defaultProps, ...props} we are overriding all props that are defined in props, even if they are defined as undefined 👀.
As an example of this behaviour, you can check that { ...{ a: 1 }, ...{ a: undefined} } evaluates to {a: undefined}.

One solution for this problem could be to clear props defined as undefined before using them, like:

function getOnlyDefinedProps(props) {
  return Object.fromEntries(
    Object.entries(props).filter(([_, v]) => v !== undefined)
  )
}

// We could also memoize the result, so that the function only runs when props change
function useDefinedProps(props) {
  return useMemo(() => getOnlyDefinedProps(props), [props])
}

function Component(props) {
  const definedProps = useDefinedProps(props)
  const readyToUseProps = {...defaultProps, ...definedProps}
 //...
}

I hope this helps, as it would fix both the failing tests for the line chart and the tooltip not showing for the choropleth chart.

@andre19980 @plouc Let me know if you want me to help with this PR.

[andre19980]

Awesome, @fernandojpps! Thank you for the help!
Your solution seems to fix the failing test, but I wonder if filtering the received props is actually the right behavior, 'cause maybe the person using the component wants to send an undefined prop. In this case, the component shouldn't discard it, especially if it's gonna affect the component's behavior, like prevent something from being rendered. Then I guess we would be creating a bug.

[ofps]

You're welcome @andre19980!

I understand your point. I actually find it weird that undefined can have two different meanings.

From my understanding, the current behavior with defaultProps is to use default values whenever a prop is undefined. It doesn't matter if it was explicitly set as undefined or not.

I created a simple React Component to test how it actually behaves, and here are the results.

const UndefinedTest = ({name = "John"}) => <p>Hello {name}</p>

<UndefinedTest name={undefined} /> //<p>Hello John</p>
<UndefinedTest />                  //<p>Hello John</p>
<UndefinedTest name="Joe" />.      //<p>Hello Joe</p>

I believe my suggestion helps keep the behavior aligned with the default React behavior, and in turn with developer expectations.

[plouc]

@andre19980, @fernandojpps,

Thank you for your help with this issue, another option would be to explicitly assign a default value for each property needing one, that's what I did for other components, I found it clear enough, even if it's a bit more tedious to maintain, it's easy to see when you have a default or not. That being said, I'm not against the suggested approach as long as it leads to the same default behavior.

[andre19980]

@fernandojpps you're totally right, my bad 🥸 . Hope you don't mind, but I followed @plouc's approach, just cause I also thought it would be more explicit and clearer

[ofps]

@andre19980 No problem! 🤓
I'm glad we figured out what the problem was and found a way to fix it!

And I agree with you, @plouc's approach makes it easier to read and understand what is going on.

[nehalist]

Any chance this is going to be merged?

[plouc]

LGTM.

[nehalist]

I'm still getting the error

Since there hasn't been made a new release since this was merged, it's not that surprising. @plouc any plans on releasing a new version in the near future? Thanks a lot!

[plouc]

@nehalist, I'll try to create a new release next week.

[nehalist]

Thanks a lot! 👍

[b310L]

When will the new version be released?

[GreenAsJade]

This PR seems to be the likely cause of #2539 . Any chance of someone "here" taking a look?

[andre19980]

hey @GreenAsJade thanks for reporting the issue! So I took a look into the code to see what may be causing the problem and found out that the Line component has its props defined with isRequired in the PropTypes declaration. One can check in the file packages/line/src/props.js

I was able to fix the warnings by removing the requirement conditions, but I'm not sure if I'm allowed to delete these protections. @plouc I saw in the documentation that many of those props are optional, so I'm a bit confused if they should be marked with isRequired. To solve the warnings, I was thinking of editing the components where I did the refactor in the default props and remove the isRequired condition from the props which are written as optional in the documentation. Would that be ok and safe?

Btw, I think this problem is also causing the bug #2077, so I think it would solve both problems

[plouc]

Properties were required because we had defaults, due to how we now define the default values, it won't pass the prop-types validation, I think we should remove the prop-types for the package, as it was done for others, relying on TS instead.
I removed the prop-types for most packages when I migrated them to TS, @nivo/line hasn't been migrated yet, but we could still remove them.

[csandman]

Properties were required because we had defaults, due to how we now define the default values, it won't pass the prop-types validation, I think we should remove the prop-types for the package, as it was done for others, relying on TS instead. I removed the prop-types for most packages when I migrated them to TS, @nivo/line hasn't been migrated yet, but we could still remove them.

That would probably be the ideal approach. Most people rely on TS for prop type validation nowadays anyway, and the errors are pretty annoying.