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?