Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Warn in dev if shouldComponentUpdate is defined on PureComponent #9239

Closed
aweary opened this issue Mar 22, 2017 · 8 comments
Closed

Warn in dev if shouldComponentUpdate is defined on PureComponent #9239

aweary opened this issue Mar 22, 2017 · 8 comments

Comments

@aweary
Copy link
Contributor

aweary commented Mar 22, 2017

Currently, if you define shouldComponentUpdate on React.PureComponent nothing bad happens and it acts like React.Component https://jsfiddle.net/jekmdfva/

We should probably warn in dev since it defaults the point of PureComponent

@gaearon
Copy link
Collaborator

gaearon commented Mar 22, 2017

Unrelated, but I also wanted to suggest warning for setting defaultProps and propTypes on instances (I've seen people do this with class properties syntax). Maybe somebody could do all three warnings together?

@aweary
Copy link
Contributor Author

aweary commented Mar 23, 2017

That's a great idea, happy to help anyone who wants to give this a shot

@misoguy
Copy link
Contributor

misoguy commented Mar 23, 2017

I'd like to give this a shot :)

@misoguy
Copy link
Contributor

misoguy commented Mar 23, 2017

What kind of warning message should it be? I'm trying to write a test first :)

@misoguy
Copy link
Contributor

misoguy commented Mar 23, 2017

After writing a simple test code, I realized that there is a test code to ensure shouldComponentUpdate can be overridden.
What is the expected behavior when it is overridden?

@aweary
Copy link
Contributor Author

aweary commented Mar 23, 2017

@misoguy I responded to your question in your PR!

@abhaynikam
Copy link
Contributor

@aweary : Issue is open since 23 days. Would like to contribute for same.

@abhaynikam
Copy link
Contributor

abhaynikam commented Apr 14, 2017

Unrelated, but I also wanted to suggest warning for setting defaultProps and propTypes on instances 
(I've seen people do this with class properties syntax). 
Maybe somebody could do all three warnings together?

@gaearon : can you help me understand what is expected behavior here? I can work on adding other two warnings. @misoguy has almost completed with the shouldComponentUpdate warning while extending from PureComponent

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

4 participants