Skip to content

refactor: remove DefeaultProps from the DatePickerIOS component#32064

Closed
Fannolo wants to merge 4 commits into
facebook:mainfrom
Fannolo:master
Closed

refactor: remove DefeaultProps from the DatePickerIOS component#32064
Fannolo wants to merge 4 commits into
facebook:mainfrom
Fannolo:master

Conversation

@Fannolo
Copy link
Copy Markdown

@Fannolo Fannolo commented Aug 22, 2021

Summary

Closes issue #31605.
This is part of a bigger issue that plans to remove defaultProps from class components in order to provide a smoother transition to functional components.

Changelog

[General] [Changed] - Remove defaultProps from the DatePickerIOS Component.
[General] [Test] - Added snapshot test for the new component

Test Plan

Compiled the rn-tester folder to check if the behavior is consistent with the previous versions.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 22, 2021
@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Aug 22, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: ff4b336

@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Aug 22, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,245,595 +14
android hermes armeabi-v7a 8,764,641 +14
android hermes x86 9,690,601 +4
android hermes x86_64 9,657,819 +12
android jsc arm64-v8a 10,881,797 +7
android jsc armeabi-v7a 9,792,111 +7
android jsc x86 10,922,219 +5
android jsc x86_64 11,530,938 +2

Base commit: ff4b336

@Fannolo
Copy link
Copy Markdown
Author

Fannolo commented Aug 23, 2021

@lunaleaps can you assign and review it? Thanks!

@yungsters
Copy link
Copy Markdown
Contributor

Thanks for your contribution!

Just in case you weren't aware, the change from defaultProps to ?? is not always safe. The value in defaultProps will only be used if a prop value is undefined, whereas ?? will coerce prop values that are either undefined or null.

I manually inspected this case and it does seem safe to switch to ??. In general, I do prefer that we switch to ?? because differences in handling of undefined and null often lead to difficult bugs.

@yungsters yungsters self-assigned this Aug 23, 2021
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@Fannolo
Copy link
Copy Markdown
Author

Fannolo commented Aug 23, 2021

Thanks for your contribution!

Just in case you weren't aware, the change from defaultProps to ?? is not always safe. The value in defaultProps will only be used if a prop value is undefined, whereas ?? will coerce prop values that are either undefined or null.

I manually inspected this case and it does seem safe to switch to ??. In general, I do prefer that we switch to ?? because differences in handling of undefined and null often lead to difficult bugs.

Thank you for your input @yungsters! I didn't know defaultProps will coerce only with undefined values. I chose to use ?? in this case because I didn't see null defined as a type accepted in the prop mode. I will definitely pay more attention in the future when replacing defaultProps with ??.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Copy Markdown
Contributor

@yungsters merged this pull request in 2fb102b.

@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants