Skip to content

Omit null state and props to fix default props#5

Merged
roryabraham merged 2 commits intomasterfrom
amechler-fix-default-props
Nov 17, 2020
Merged

Omit null state and props to fix default props#5
roryabraham merged 2 commits intomasterfrom
amechler-fix-default-props

Conversation

@alex-mechler
Copy link
Contributor

@alex-mechler alex-mechler commented Nov 13, 2020

Part of https://github.com/Expensify/Expensify/issues/144333

React will only replace undefined props with defaultProps. We want to omit any state or props that are null when passing them to wrapped components to fix default props.

Test with Expensify/App#795

cc: @roryabraham @marcaaron

@alex-mechler alex-mechler self-assigned this Nov 13, 2020
@alex-mechler alex-mechler marked this pull request as ready for review November 13, 2020 21:49
@roryabraham
Copy link
Contributor

Looks good, but why remove null state instead of just null props?

@alex-mechler
Copy link
Contributor Author

alex-mechler commented Nov 16, 2020

Looks good, but why remove null state instead of just null props?

We are passing the state of the withOnyx wrapper as props, not as state, to the wrapped components. If there is a null value in the withOnyx wrapper's state, it will be passed as a null prop, breaking defaultProps.

I actually ran into this breaking a component while testing (I think it was the typing indicator

@roryabraham
Copy link
Contributor

Ohh gotcha. Thanks, that was super obvious looking at it again.

This PR looks good to me, but I'm going to hold off on merging it because there were some issues I ran into when testing Expensify/App#795. I'll post about them over there.

@roryabraham
Copy link
Contributor

Okay, issues were resolved over there so I'm merging.

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