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: remove deprecated defaultProps #277

Closed
wants to merge 1 commit into from

Conversation

iwfan
Copy link
Contributor

@iwfan iwfan commented Jun 27, 2023

The defaultProps are being deprecated by React: reactjs/rfcs#107, facebook/react#16210

So remove defaultProps to avoid warnings from React

The `defaultProps` are being deprecated by React,
So remove defaultProps to avoid warnings from React
@iwfan iwfan requested review from romac and favna as code owners June 27, 2023 13:55
@favna
Copy link
Collaborator

favna commented Jun 29, 2023

This is the wrong fix because you're no longer spreading in the default props. The proper fix is in #279

@favna favna closed this Jun 29, 2023
@iwfan
Copy link
Contributor Author

iwfan commented Jun 29, 2023

This is the wrong fix because you're no longer spreading in the default props. The proper fix is in #279

I do not think so. undefined also is a Nullish value in JavaScript. And the render function is perfectly capable of handling nullish value.

react-if/src/render.tsx

Lines 8 to 14 in c8c0866

export const render: FCWithImplicitChildren = (props) => {
if (typeof props.children === 'function') {
return <Fragment>{props.children()}</Fragment>;
}
return <Fragment>{props.children || null}</Fragment>;
};

So explicit declare null as default value of children seems redundant.

@favna
Copy link
Collaborator

favna commented Jun 29, 2023

null and undefined may both be nullish yes but they are still distinct. Furthermore, removing it entirely would be a breaking change (major version) whereas Kaname's change is merely a fix (patch version).

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.

2 participants