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

React doesn't repeat propType warnings for multiple renders of same named component. #7047

Closed
joejuzl opened this issue Jun 15, 2016 · 9 comments

Comments

@joejuzl
Copy link

joejuzl commented Jun 15, 2016

Bug:

When React renders a component with invalid props, it logs a warning.
If the same named component is rendered again, with the same invalid props, there is no warning.

Demonstration:

https://jsfiddle.net/x5b9u8ow/2/

Why it's an issue for me:

When I am testing my react components with jasmine, karma and webpack all components end up being referred to as t (I am not 100% sure why).
This means that when I am testing propType validation, subsequent tests that have different components with the same named prop will not log warnings.

@joejuzl
Copy link
Author

joejuzl commented Jun 15, 2016

Update

The line causing this: source

I guess this is intended behaviour then?
IMO it seems wrong...

Any suggestions on how to test propTypes given that the warnings get suppressed would be great.

@aweary
Copy link
Contributor

aweary commented Jun 15, 2016

I believe it only logs it once so that it doesn't keep logging the errors over and over every time a component is rendered. I would try and investigate why all your components are being referenced as t instead, as that seems to be the actual problem here.

@gaearon
Copy link
Collaborator

gaearon commented Jun 15, 2016

When I am testing my react components with jasmine, karma and webpack all components end up being referred to as t (I am not 100% sure why).

Maybe you’ve enabled minification in tests?

@jimfb
Copy link
Contributor

jimfb commented Jun 15, 2016

Yeah, I'm going to close this out, because the deduplication of error messages is intentional to avoid log spew. There isn't anything actionable on our end. The error messages wouldn't have been super useful anyway, if all the component names are t, so that seems like the first problem to solve. Once you've fixed your unit test configuration, I assume the deduplication won't be a problem anymore.

Also, one other recommendation is to use strong test issolation, such that React is reinitialized for every test. That will avoid any issues where one error message gets eaten up by a previous unit test. It will also make your unit tests less flakey in general, so it's a good idea.

Feel free to continue the discussion on this thread.

@jimfb jimfb closed this as completed Jun 15, 2016
@joejuzl
Copy link
Author

joejuzl commented Jun 15, 2016

Fixed the test by disabling minification as suggested above.
Still seems strange to dedupe errors in the code though.... Especially since chrome DevTools console already combines identical messages.

@jimfb
Copy link
Contributor

jimfb commented Jun 15, 2016

Especially since chrome DevTools console already combines identical messages.

Suppose the component/app emits two warnings. Then devtools can't combine them and there will be warning thrash. Or suppose the client is doing some debugging logging and these warnings (from multiple different components) will constantly be trying to interject themselves. It gets annoying. Thus we deduplicate.

@saifelse
Copy link
Contributor

Also, one other recommendation is to use strong test issolation, such that React is reinitialized for every test. That will avoid any issues where one error message gets eaten up by a previous unit test. It will also make your unit tests less flakey in general, so it's a good idea.

As nice as strong test isolation is, it can add significant overhead to testing, e.g. you have to use something like proxyquire so that each test in the same file gets a new React, or maybe you blow it away from the modules cache which would only help per file... these measures can slow down test writing time and running time. Empirically, I would say most packages that I use are stateless, so reinitializing modules is often unnecessary and impractically makes tests slower.

Exposing something like:

React.resetWarnings = function(){
 Object.keys(loggedTypeFailures).forEach((key) => {delete loggedTypeFailures[key]});
}

which you call after each test, would be super simple, fast, and easy to use.

@gaearon
Copy link
Collaborator

gaearon commented Jun 26, 2016

As nice as strong test isolation is, it can add significant overhead to testing, e.g. you have to use something like proxyquire so that each test in the same file gets a new React, or maybe you blow it away from the modules cache which would only help per file

The (ugliyish) strategy I’ve been using in personal project is something like

MyComponent.displayName = Math.random().toString()

before every test. This helps avoid caching but obviously it’s far from ideal 😉 .

In the future, with official DevTools API (#5306), it should be possible to isolate warnings in tests better. But we’re not there yet. We’re keeping it in mind though.

@jakemmarsh
Copy link

Just ran into this same issue trying to test custom prop type validation against multiple similar scenarios. Currently using the displayName hack mentioned by @gaearon, but definitely not ideal!

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

No branches or pull requests

6 participants