facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.13k stars 46.3k forks source link

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

Closed joejuzl closed 8 years ago

joejuzl commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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.

joejuzl commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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!