react: Bug(regression): PropTypes.resetWarningCache has no effect in react@next

React version: 0.0.0-235a6c4af

Steps To Reproduce

render component with exptected propType warnings
see logged errors
call PropTypes.resetWarningCache()
render component with exptected propType warnings
no more logged errors

Link to code example:

import React from "react";
import ReactDOM from "react-dom";
import PropTypes from "prop-types";

function Component({ children = "setting default to not crash react" }) {
  return children;
}

Component.propTypes = { children: PropTypes.node.isRequired };

const rootElement = document.createElement("div");

ReactDOM.render(<Component />, rootElement);
PropTypes.resetWarningCache();
ReactDOM.render(<Component />, rootElement);

Behavior on 0.0.0-235a6c4af Behavior on 16.13.0

The current behavior

Warnings are logged once

The expected behavior

Warnings are logged twice. This was the behavior in 16.13.0

Context

Useful for testing custom propTypes warnings. When a test is watched and modules can’t be reset between runs resetWarningCache was useful. Otherwise the test behavior would change between test runs.

Pretty sure this is caused by inlining prop-types in #18127. I think we can keep inlining the code and only import the cache so that it is shared with the regular prop-types package and PropTypes.resetWarningCache() keeps working.

About this issue

Original URL
State: closed
Created 4 years ago
Reactions: 1
Comments: 28 (10 by maintainers)

Most upvoted comments

This is a bit unfortunate. But to be honest, we didn’t really sign up to respect this setting in React itself. The fact that the prop-types module used by React was technically shared is an implementation detail that could easily change, e.g. if we pinned our version.

Useful for testing custom propTypes warnings. When a test is watched and modules can’t be reset between runs resetWarningCache was useful. Otherwise the test behavior would change between test runs.

While I probably won’t convince you, jest.resetModules() in beforeEach + CommonJS in tests does this too. That’s what we’ve been using. It works pretty well and resets other state you might want to make isolated.

I think we can keep inlining the code and only import the cache so that it is shared with the regular prop-types package and PropTypes.resetWarningCache() keeps working.

The purpose was to get rid of imports.

I’m still not really clear on the value of having fewer dependencies, especially in the case of prop-types, which facebook controls.

Prop-types is a legacy feature. It’s not something that is super important to React going forward. We try to keep it working, but it’s not a first-class “React API”. There is no need for us to keep the implementation separate when the contract is so simple and we could just call into the user code from React here.

On the other hand, changes related to ESM and just our bundling process is always more complicated when there’s dependencies involved. We have so few dependencies that this isn’t worth the complexity. It is easier to get rid of them.

gaearon on Apr 1, 2020

I’ll ping you in the PR where we migrate from .propTypes to checkPropTypes since I want to go over these custom propTypes myself just in case they are actually not necessary.

See https://github.com/mui-org/material-ui/pull/20451

eps1lon on Apr 7, 2020

function Button(props) { checkPropTypes(props, Button.propTypes) // your code }

So like a React.error? 😉

Anyway this is at least resolved for me personally since we can leverage checkPropTypes.

Can you link me to components and tests in question?

I’ll ping you in the PR where we migrate from .propTypes to checkPropTypes since I want to go over these custom propTypes myself just in case they are actually not necessary.

eps1lon on Apr 4, 2020

@gaearon because the best way to test propTypes isn’t to unit-test an individual custom propType, it’s to attempt to render a component, and fail your tests when React issues propType warnings. that, specifically, is the primary benefit of resetWarningCache - so that React’s propType warnings can be caught, and then reset, so tests in watch mode - or simply, two tests failing with the same propType failure - will all fail deterministically.

ljharb on Apr 4, 2020

The work point of moving prop-types to a separate package was so React didn’t have to expose it. Will React be exposing it now?

React won’t be exposing the PropTypes validators themselves. At the time, it seemed to make sense to use checkPropTypes from the package (since the package needed it anyway as a standalone utility), but I don’t think it makes sense now given the contract is already stable.

if it works and you break it, you’re breaking people - in a non-major. Regardless of technicality debates wrt semver, that seems like a pretty user-hostile position to take for some kind of vague maintainability benefit for the react team.

We have a versioning policy here which notes that we don’t consider changes to DEV-only behavior like warnings to be breaking. In general case, this is because warnings are how we prepare people for other major changes. I agree this isn’t exactly the case that section is meant to cover in spirit, but I hope it explains why we consider this change acceptable.

If test suites are written specifically to check React’s console output, they will break because we add new warnings, reword warnings to be clearer, sometimes remove them, change deduplication heuristics, fix bugs, and so on. If every behavior change to warnings was considered breaking, we’d be incrementing majors every week, and I doubt the ecosystem would be better for it.

My suggestion is to either not write tests that assert on console output, or be prepared that changes to these tests will not follow the same schedule as changes to production behavior. We’ve reiterated this multiple times so this isn’t really a new stance. It’s part of how we release React.

gaearon on Apr 2, 2020

This will break many test suites.

If you require prop-types in your test, there is no guarantee it would be the same instance as the one used by React. Because we could pin the version. With stricter workflows like Plug’and’Play it wouldn’t even be possible to do that. So any tests written this way were essentially relying on internals.

One can always call checkPropTypes directly if they want to check that something matches a prop type. So it’s fixable. Yes, this is unfortunate if someone already relies on this, but React never “signed up” to support this API for its own checks, and I think it’s fair to say it worked by accident.

Could React expose a way to reset the warning cache?

This is actually interesting because it exposes another problem with this approach. Even if a library “resets” the propTypes cache, that doesn’t help for any other React warnings. So it doesn’t actually solve the problem for libraries that want to verify that React warnings aren’t firing for it. You still need a better solution. Again, resetModules() is one way to do it.

Maybe there could be some centralized cache for deduplication that would work for all warnings. And React could pass it to prop-types too. That sounds like a bigger refactoring. Might be worth a separate issue if you have a plan!

gaearon on Apr 2, 2020