Add hooks into the warning output for unit tests and other use cases

[glenjamin]

I'd like to configure my testsuite to fail if there are any react warnings triggered.

I'm currently using a variant on this SO answer http://stackoverflow.com/questions/29651950/karma-and-react-have-warnings-to-cause-errors

console.warn = (function(warn) {
  return function(msg) {

    // Detect react warnings & error
    if (/^Warning: /.test(msg)) {
      throw new Error("React " + msg);
    }

    return warn.apply(this, arguments);
  };
})(console.warn);

This basically works, but has a problem because there's some global-state memoisation in the code which triggers warning to try not to trigger too often:

react/src/isomorphic/classic/element/ReactElementValidator.js

Line 46 in 500d4c3

var ownerHasKeyUseWarning = {};

Could we provide a way to clear this state, or even better a supported API for opt-in erroring on warnings?

[jimfb]

I feel like you really should solve this through unit test isolation. Each unit test should run in a new javascript context, such that nothing one test does could possible influence the other test. For this reason, I'd say a fix for this is a low-priority use case at best.

We don't want to have any global React configuration states, so supporting opt-in erroring/warnings is probably not something we're going to do.

Also related: #3252, because having multiple instances of React (presumably each with their own warning memoization/deduplication) would mean that each would fire the warnings (it's a poor-man's version of test isolation).

[glenjamin]

I feel like you really should solve this through unit test isolation. Each unit test should run in a new javascript context, such that nothing one test does could possible influence the other test.

While I can see this argument from a purity point of view, this does not reflect the way React is used in practice. Last I checked even Jest does not use an entirely separate context per test - only per file.

It's quite common for people to test using mocha or jasmine, or even with full browsers via karma or similar - spinning up an entirely new browser per test seems to me like a poor developer experience for getting fast feedback through a testsuite 😝

We don't want to have any global React configuration states, so supporting opt-in erroring/warnings is probably not something we're going to do.

This problem only arises because of some global state which we can't touch, and React's tests themselves do the same sort of check I'm doing here (see #4223).

I also realised that this doesn't just apply to testsuites, it applies to any long-running React application. This includes an application being developed via hot-reloading - you'll only ever get a warning once per browser-reload, even if an issue recurs later in the session.

It would be very helpful to have some way of clearing the global warnings state - or perhaps the cache could be changed to be once-per-render instead of once-per-javascript-context?

[jimfb]

@glenjamin The existence of low quality test frameworks is not a reason for us to compromise our designs. There are plenty of good options for test isolation, like jsc, that don't require spinning up browsers.

Long running applications are exactly the reason we warn only once per javascript context. Once we've informed the developer they have a bug, we don't need to keep warning them. Warning once per render wouldn't make sense either; we want to get to the point where we can do 60 renders per second. Moving forward, we expect renders to happen more (not less) frequently.

The argument for things like hotloading is a little harder to refute. While I personally don't use hotloading, I can see why it's useful and why allowing the hotloader to reset React would be desirable. But I think the solution for hotloading is a combination of #3252 and #3932 (comment)

[glenjamin]

It seems a bit heavyweight to me to bundle this up into a "remove all state from react" thing.

This particular hidden global state is introduced to improve the developer experience in one aspect (not getting spammed with warnings), but IMO is having a negative impact on another aspect (being able to force warnings across multiple renders).

If the goal in general is to remove global state from React, could we come up with a path to achieve this particular bit?

I dug around a bit and found that this is sort-of a dupe of #3854 - although i'm mostly talking about different use-cases.

[jimfb]

Yep, it's a tough problem. We've talked about the idea of adding a clearAllInternalState() function, which might be a possible solution. I still think the best solution to your particular use case is test isolation, but I understand that's not possible in all setups/environments/situations.

I'm leaving this thread open for tracking purposes. We don't really have a good solution at the moment. Feel free to let us know if you have ideas / proposals for solving this.

[glenjamin]

One possible option might be to move the "no-duplicate-messages" logic into the (or a wrapper) warning module instead of the custom code in the ElementValidator.

Instead of calling warning(XXX) we'd call warnOnce(XXX), and then there's a clear injection point for either hijacking the warning function in tests, or having some reset() in the warning module.

[jimfb]

Yep, I actually created a diff for that exact idea a few months ago (not sure if it ever hit github, but I do know we ultimately decided to abandon it). The problem was that the deduplication logic was often times specific to the use case (Do you warn once globally? Once per component? Once per property name? Back off logarithmically based on time?) - it was too difficult to generalize without it being too awkward to add warnings. That doesn't mean it isn't possible though, just harder than it sounds at first glance.

So far, I most like the idea of providing some kind of reset functionality to addons.

[glenjamin]

I guess part of the problem is that without moving the state logic to a (fairly) central place, it's quite tricky to reset it all at once.

If it stayed distributed throughout, there'd need to be some object to co-ordinate all the resetting through?

[jimfb]

Yeah, you'd need some way of coordinating the intent to dump the state.

Incidentally, I just realized a possible solution might be to pass an extra argument to the warning module (specifically, a boolean, to indicate if React thinks the error message should be shown based on our deduplication rules). The warning module we provide can honor the boolean and only print if the value is true, but power users could override the module to implement their own preferred deduplication logic or ignore the deduplication flag altogether. I don't really see any meaningful downsides to this approach.

[glenjamin]

I like this idea! Logic for whether it's a repeat stays where it is, but the warnings module gets to decide what to do with that information.

I should be able to put together a PR for this in the next few weeks (when I get back from holiday)

The slight kink is that the warning function currently takes varargs, so there's no space for any additional arguments.
https://github.com/facebook/react/blob/master/src/shared/vendor/core/warning.js#L26

How would you feel about having an additional export from this module, something along the lines of:

warning = function(condition, format, ...args) {
// ...
}
warning.once = function(condition, isARepeat, format, ...args) {
// ...

[jimfb]

I don't mind inserting the boolean between condition and format as you indicated, but @zpao is usually the one with the strongest opinions on modifying function signatures, so just run it by him before investing time in a PR.

[glenjamin]

Just had a look at what would be involved in implementing this now, the internals have moved around a bit, so it's slightly more fiddly now.

• Overload warning() to take an optional boolean as the second argument to indicate that the caller thinks this warning is a repeat of a previous warning (in the fbjs lib)
• Update relevant callsites in React to pass this boolean where they have dedupe logic
• Update the eslint rule which checks args of warning to allow the new signature
• (possibly) provide a supported way to inject custom warning, rather than having to require('fbjs/lib/warning') and somehow modify it

Is this something you'd be open to accepting @zpao ? it seems related to facebook/fbjs#14

[joeybaker]

@jimfb @zpao any thoughts on accepting a PR here?

[jimfb]

I'll defer to @zpao here, I have no objections.

[jimfb]

Ping @zpao

[joeybaker]

@jimfb I'm trying to think of a good API here. I see that @glenjamin suggested a way to overload warning(), which seems like a decent idea. What do you think of an API like:

React.onWarning = (warningMessage) => {
  // to keep the default behavior
  require('fbjs/lib/warning')(warningMessage)

  // or, to change behavior
  throw new Error(warningMessage)
}

[jimfb]

@joeybaker We have worked very hard to avoid all global configuration, and probably aren't going to add it here, so I don't think we're ever going to expose React.onWarning.

Another unrelated update: I was talking with Sebastian the other day, and we were discussing design ideas around a good devtools API. Specifically, we want to expose an API that could power tools like https://facebook.github.io/react/blog/2015/08/03/new-react-devtools-beta.html . Anyway, the related chunk of that discussion is that it might be possible to move all the warning-related logic out of the core and into an external module (to be packaged with React in the dev distribution) that uses the same devtools API to figure out and emit warnings. With that refactoring, you could easily fork that module and do whatever you wanted with regards to warnings, since none of the warning logic would live in the core.

[joeybaker]

That sounds great! What would that API look like?

[scottnonnenberg]

Here's another scenario supporting the need for this:

• React component tests which verify that propTypes are calling console.error() given certain props
• running the tests in Node.js via mocha --watch

The first run, everything works fine. When a file is modified, all of the propTypes tests fail, and will continue to fail until the process is restarted.

Would really appreciate a hook to force the log or some sort of ability to reset all that data when a test run starts.

[stoikerty]

Does anybody know of a workaround to get the --watch scenario mentioned by @scottnonnenberg working? Unfortunately @gaearon's workaround is not applicable as the warnings are still only triggered once.

I have a custom PropType, the only solution I can think of is to test that method separately. I feel though that it defeats the purpose of testing how the component is going to be used.

[bvaughn]

I realize I'm replying to a comment that's over a year old, but... 😄

Does anybody know of a workaround to get the --watch scenario mentioned by @scottnonnenberg working? Unfortunately @gaearon's workaround is not applicable as the warnings are still only triggered once.

I just did a quick test (below) and Dan's hack-around does still seem to work so far as I can see, although I realize it's far from ideal. For example:

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

const CustomComponent = () => null;
CustomComponent.propTypes = {
  requiredBool: PropTypes.bool.isRequired
};

let counter = 0;

beforeEach(() => {
  CustomComponent.displayName = `CustomComponent-${++counter}`
})

it('test 1', () => {
  const div = document.createElement('div');
  ReactDOM.render(<CustomComponent />, div);
});

it('test 2', () => {
  const div = document.createElement('div');
  ReactDOM.render(<CustomComponent />, div);
});

The above test will result in the following warnings:

  console.error node_modules/fbjs/lib/warning.js:33
    Warning: Failed prop type: The prop `requiredBool` is marked as required in `CustomComponent-1`, but its value is `undefined`.
        in CustomComponent-1 (at App.test.js:20)

  console.error node_modules/fbjs/lib/warning.js:33
    Warning: Failed prop type: The prop `requiredBool` is marked as required in `CustomComponent-2`, but its value is `undefined`.
        in CustomComponent-2 (at App.test.js:25)

[gaearon]

I'd suggest just using PropTypes.checkPropTypes() in unit tests instead these days.

[gaearon]

Never mind my last comment, it's wrong. checkPropTypes() doesn't return the failures (and it also deduplicates them). Maybe we should expose something there that doesn't.

[dtinth]

Hello, any updates on this? Solving this would resolve these issues:

• Yellow Box for react-dom #7360 — “Yellow Box for react-dom” still isn’t implemented, and it is still not possible for userland code to implement that functionality (*).

• Feature request: Warnings hook #10072 — My own use case, which still isn’t possible (*), but was closed as a duplicate of this issue.

(*) Without hacking the module system to replace fbjs/lib/warning or overriding console.error which breaks the console log stack trace.

I am willing to work on solving this issue but I would like to hear how React team would like this issue solved.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!