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

[eps1lon]

React version: 0.0.0-235a6c4af

Steps To Reproduce

1. render component with exptected propType warnings
2. see logged errors
3. call PropTypes.resetWarningCache()
4. render component with exptected propType warnings
5. 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.

[threepointone]

importing the cache would defeat the purpose of inlining, which is to have fewer dependencies. Tricky.

[ljharb]

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

[gaearon]

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.

[ljharb]

This will break many test suites.

Could React expose a way to reset the warning cache? prop-types could use that rather than its own mechanism.

[gaearon]

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!

[ljharb]

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?

The test suites I’m concerned about don’t care about non-propType react warnings.

[ljharb]

Separately, whether it works by accident or not isn’t really relevant; 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.

[ljharb]

I would also point you to https://www.w3.org/TR/html-design-principles/#priority-of-constituencies, something that’s generally considered a good principle in all software design - namely, that users always matter more than authors.

[gaearon]

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.

[ljharb]

There's a critical difference here - non-propType warnings are about misusing React, and I think it's reasonable to consider this non breaking.

propType warnings, however, are about misusing a component, and this does not seem to me to be in the same category.

[eps1lon]

If test suites are written specifically to check React's console output

Just to give a bit more background how we're using this:

We're not checking react's console output (or at least we don't mind rewriting tests if those change). However, Component.propTypes supports custom prop-types which are officially documented: https://reactjs.org/docs/typechecking-with-proptypes.html#proptypes (customArrayProp). We use them for deprecation warnings, apply heuristics to determine if a component is a able to hold a ref, or generally refine a propType further (since prop-types only supports the or operator not and operator).

The ability to test these validators is now severly limited because watchmode doesn't work for them. This means working on them takes much more work (since I have to restart the test every time) which means I'm less like to work on them to improve DX.

The dilemma here is that I don't think propTypes is the right choice for these. I would've preferred React.warn and React.error but these were also stripped

What makes this stance come across so user-hostile is that this wasn't done to enable something else (fix a bug, add a feature, improve perf). It was only done because you could and is then dismissed with "just use jest". Apart from the cost of having to migrate to another test runer, jest is also lacking a critical feature over other runners: it can't run in the browser.

Would a React.resetPropTypesWarningCache work for you? This would ensure that the correct cache is reset (in case React#propTypes and propTypes has a version mismatch.

[gaearon]

Let’s step back for a second. Can you explain in more detail why explicitly calling checkPropTypes doesn’t work for you? That should still work with resetting like before.

[gaearon]

The dilemma here is that I don't think propTypes is the right choice for these. I would've preferred React.warn and React.error but these were also stripped

console.error is still there. Why don’t you use that?