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

Avoid to set refs on stateless components #866

Merged
merged 1 commit into from
Jun 6, 2017
Merged

Avoid to set refs on stateless components #866

merged 1 commit into from
Jun 6, 2017

Conversation

farwayer
Copy link
Contributor

@farwayer farwayer commented May 30, 2017

fix #865 #750

@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@msftclas
Copy link

@farwayer, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, Microsoft Pull Request Bot

Copy link

@chirag04 chirag04 left a comment

Choose a reason for hiding this comment

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

Maybe it's better explicitly ask for withRef option like how redux connect does

@farwayer
Copy link
Contributor Author

Ask for what? ref used internally only to make codePushStatusDidChange and codePushDownloadDidProgress functions work. With stateless components it is senselessly.

@chirag04
Copy link

Set the ref only if withRef option is provided and check if ref exists before using it in any function

@max-mironov max-mironov self-requested a review June 2, 2017 11:38
@max-mironov
Copy link
Contributor

Hi @farwayer , thank you so much for contribution, your PR looks good for me, I've also tested it one more time and haven't found any issues with this.

@chirag04 - could you please explain a little bit more your point - what are the benefits here with adding withRef option in such particular case?

@max-mironov
Copy link
Contributor

Guys - I believe we can merge this changes to Master and we can always enhance this in future if needed. Please feel free to voice your concerns or suggestions if any.

@max-mironov max-mironov merged commit 3d5bf1a into microsoft:master Jun 6, 2017
@farwayer farwayer deleted the ref-class-only branch June 7, 2017 11:03
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.

CodePush should avoid set ref if stateless component wrapped
4 participants