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?

[eps1lon]

Can you explain in more detail why explicitly calling checkPropTypes doesn’t work for you?

I wasn't aware of that method. But apparently it only fires once:

PropTypes.checkPropTypes(...) only console.errors a given message once. To reset the error warning cache in tests, call PropTypes.resetWarningCache()

-- https://github.com/facebook/prop-types#proptypesresetwarningcache

This would break our tests in watchmode. We never had a use for resetting modules between tests since the modules under test were stateless (at least the parts we wanted to test). It's also not trivial to do in mocha (requires 3rd party package).

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

It does not include the component stack on the server nor can we deduplicate the warning by component stack.

[eps1lon]

Previously we were appending a random string to the message for cache busting but that hurt readability. We'll probably go back to that approach and hide it in a custom matcher. I'm just a bit worried that checkPropTypes will also go away in a minor with the same justification.

[gaearon]

Before you assume malice, please help me understand the issue better.

But apparently it only fires once:

PropTypes.checkPropTypes(...) only console.errors a given message once. To reset the error warning cache in tests, call PropTypes.resetWarningCache()

Yes, so you need to call PropTypes.resetWarningCache(). We didn't break it.

The only change is that PropTypes.resetWarningCache() wouldn't reset warnings coming from React.

If you call PropTypes.checkPropTypes() in your tests, PropTypes.resetWarningCache() would reset those just fine.

Can you please explain why this doesn't work for you?

I'm just a bit worried that checkPropTypes will also go away in a minor with the same justification.

I'm sorry that you got the impression that we don't care about our users and are going to remove APIs in minors. I don't believe this is what happened here. We didn't remove any API in a minor, and don't intend to.

PropTypes itself is completely separate from React. We don't intend to even touch that package in the future. If you want to help maintain it, you're welcome to become a co-maintainer. If you only use its APIs (e.g. checkPropTypes + resetWarningCache) everything still works.

The only thing we "removed" is the ability for resetWarningCache to influence what React prints. Because that was never something we signed up for. resetWarningCache was added by a third-party contributor to prop-types (that wasn't a feature we developed while it was in React), and at no point in time did we say that React would abide by that feature. It happened to work by accident, but even then in limited circumstances (e.g. it would break with version pinning). So we don't consider this a breakage.

Given that the alternative (checkPropTypes + resetWarningCache) exists, has a simpler API, and should continue working indefinitely, can you explain why it doesn't work for you?

[ljharb]

@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.

[eps1lon]

Can you please explain why this doesn't work for you?

If that is indeed the case then it would work for me. I simply assumed that resetWarningCache works the same for .propTypes and checkPropTypes. Let me try checkPropTypes and then I'll update the documentation since I don't think it's obvious.

In a perfect world I wouldn't unit test propTypes with resetWarningCache but that isn't that important to me. I'm more interested in a practical compromise than some perfect theoretical approach.

[eps1lon]

It technically works for us but we have to mock a couple of internals:

PropTypes.checkPropTypes(
  ButtonBase.Wrapped.propTypes,
  { classes: {}, component: Component },
  'prop',
  'ForwardRef(ButtonBase)',
);

1. The component under test still uses higher-order components which means we have to inject props from the HOC manually (here classes)
2. Since it is wrapped we have to access the internal component (here ButtonBase.Wrapped)
3. The component name doesn't really matter but it would've been nice to see if what displayName the user gets to see.

So this works for us but there are quite a bit of internals we have to mock.

Could you explain why it wouldn't work for you to expose resetPropTypesWarningCache from react?

[gaearon]

Could you explain why it wouldn't work for you to expose resetPropTypesWarningCache from react?

It is a new API. We don’t add new APIs lightly. For the ones we do, we want them to stick around for a long term.

I don’t know if even calling PropTypes at all is something we want to support in longer term. They are a relatively poor replacement for type checking and have nothing to do with UI. We understand many people care about them, but it’s pretty arbitrary that you would limit runtime type checking to UI alone. Why not all modules? The point of extracting them was in part to make that easier and more natural. So I imagine in distant future React may stop calling them and instead there would be a codemod that turns your components into:

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

Effectively fully moving them into userland.

With a vision like this, it doesn’t make sense to add more propTypes-specific APIs to React itself.

[gaearon]

It technically works for us but we have to mock a couple of internals:

Can you link me to components and tests in question? I’m low key wondering why you want to test prop types at all. They’re already fully declarative so it seems like writing the same code twice. Unlike components, they (usually) don’t contain any logic so the only way to mess them up is to have typos. Which would be caught by a shallow test asserting their presence with given names. As long as your other tests (for UI) don’t trigger warnings, I don’t know what it is that you want to be testing.

I understand the need to test a custom propType validator but that can be done in isolation easily.

[gaearon]

I think my overall conclusion is that I see some value in being able to reset all warnings, but I struggle to see value in “testing propTypes” because they don’t contain any custom logic. The only kind of test I can imagine being valuable is making sure legit use of your component doesn’t trigger them. But you already get that even if you don’t reset warnings. I agree non-determinism is best avoided in tests, but that logic applies much more importantly to other React warnings that point out real bugs. So if you care about this you either need resetModules() or an equivalent. Such as delete require.cache[...] might work in Node. If neither of these alternatives suffices then I guess a React RFC for a way to reset all warnings would be in order so we can talk about how that could (or could not) work. But it doesn’t make sense to me to solve this for propTypes alone.

[eps1lon]

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.

[ljharb]

Deleting the require cache doesn't work in node, unless you delete all of the cached code of your entire graph - because modules cache things at module level.

resetModules only works because jest hijacks node's module system (creating many bugs and inconveniences in the process); it's not reasonable to recommend that universally.

Separately, propTypes are better than types for most cases - you can propType an integer, or strings that match a regex, for example, none of which you can do in TS. propTypes and types are complementary, and it's simply false to claim that propTypes are a poor replacement for type checking.

[gaearon]

Deleting the require cache doesn't work in node, unless you delete all of the cached code of your entire graph - because modules cache things at module level.

React compiles to bundles. So deleting react-dom/cjs/react-dom.development.js from the cache should be sufficient to reset React warnings if you really want to. Again, I'm not saying it's "recommended", but since React doesn't offer a first-class way to reset warnings, that's the best I can offer.

Separately, propTypes are better than types for most cases - you can propType an integer, or strings that match a regex, for example, none of which you can do in TS.

Sure. In my experience of looking at tens of thousands of components both in open source and at FB, this powerful capability is rarely used in practice. For the cases where runtime validation specifically is important, manually calling PropTypes.checkPropTypes() or a custom equivalent would work well. And it works for non-components too, which seems useful if runtime validation is important for your use cases.

What I'm saying isn't that PropTypes are useless. It's more that most of their use cases have been superseded, and the remaining use cases aren't tied to React so a general solution would make sense.

[eps1lon]

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 mui/material-ui#20451

[ljharb]

@gaearon since every component in every app everywhere does import React from 'react', and that reference is cached in its jsx, deleting React from the module cache is actually entirely ineffective, and would result in two copies of React in the system if you re-imported from react. With the change you're making here, the only solution is "reset everything and reimport everything on every test", which i suspect would decimate any speed advantages jest provides.

[gaearon]

Ah right. I forgot this happens in the React bundle rather than ReactDOM. (Where the majority of warnings lives.)