Add fail-fast mode to React

[slorber]

When using components with missing/invalid prop, then a warning is logged.
Same when a child key was not provided.

I would like an error to be thrown instead.

This has already been discussed on some issues like: #1587

By the way, I totally support this related issue, to ensure proper Proptype documentation of all components, but would require an error instead of a warning :)

The more we fail in dev, the better. It would be nice to be able to use React in a very strict way, by using some configuration attribute or something...

One other thing I would probably add is to enforce all components have a name.
See #1751

[syranide]

👍 from me at least, I understand wanting identical behavior in DEV and PROD, but to me it seems it would only be a problem if it was the other way around (error in PROD and warning in DEV). Too many times have I found myself troubleshooting an issue already printed in the console.

Although there's a problem with it being an option too, third-party modules are not guaranteed (or even likely) to strictly follow your current definition of what is mandatory. So unless it's per file/module somehow, it would be problematic.

[sophiebits]

I think it's likely that we'll do this, possibly as the default behavior.

[slorber]

nice @spicyj

Also I thought React used Object.freeze on props and state once set but it doesn't seem to be the case like we've seen with @syranide on IRC

In fail-fast / strict mode / dev it would be nice to freeze props (deeply) and state so that we can be sure no component is actually updated in place. The drawback is that to really enable fail-fast with this, this requires the user to add use strict mode everywhere or the mutable updated are simply ignored without any error thrown

[syranide]

@slorber While it's arguable that it is bad practice to change state in-place, React does not enforce immutability, it only (strongly) encourages it.

[slorber]

@spicyj What about failing fast being the default, and be able to disable this behavior on a per file basis (like suggested @syranide)

[davidtheclark]

I'm a huge fan of this idea. It is definitely what I want to enforce in my project.

I have tried manually freezing my props objects, but there are various React internals that can end up cloning the object and thereby nullifying the freeze. It would be very nice to be able to simply opt-in to frozen props.

[slorber]

@davidtheclark I'm using freeze myself and did not have any problem.

Note that freezing is not recursive, so if you freeze this.props directly it actually does not freeze any of your properties but the root object only. As far as I know the props object is recreated on each component render btw.

I use this:
https://github.com/stample/atom-react/blob/master/src/utils/deepFreeze.js

[gaearon]

I didn't realize freeze wasn't recursive, thanks for pointing that out.

[davidtheclark]

Thanks @slorber. Deep freezing isn't the issue I'm having, but React internals cloning the object I'm passing in. When are you freezing the object? Within the component, or before it is passed to the component (as spread or through a factory)?

The situation I'm referring to is that when I pass in a frozen object to become props -- through spread in JSX or an element factory -- it is exposed to the component unfrozen.

I see two places that probably cause this:

In ReactElement.createElement() (https://github.com/facebook/react/blob/master/src/classic/element/ReactElement.js#L147) the config parameter is the object I passed in, the passed-in-props-object, which was what I intended to become my component's this.props. But createElement() actually creates a new object, the variable props, which is assigned all the properties of config and then it is what becomes my component's this.props -- not my original, frozen object.

So if I have recursively frozen my passed-in-props-object, what comes out and is available in the component's methods, the component-props-object, this.props, is a new, unfrozen object with the same properties as the original object I passed in. If one of my passed-in-props named choices were an array that had been frozen, the component-prop choices would be that same, frozen array (because the reference was passed to the cloned object); but the this.props object itself is no longer frozen, so all the primitive properties that I passed in can still be mutated without throwing an error (in strict mode). Does that make sense?

Then essentially the same thing happens when passing JSX spread attributes because the spread implementation creates a new object that is assigned the properties of the object you pass in: https://github.com/facebook/react/blob/master/src/browser/ui/React.js#L82

The way I'm patching this now is a simple mixin, re-freezing props within the component, after they've gone through ReactElement.createElement():

var FreezePropsMixin = {
    componentWillMount: function() {
        Object.freeze(this.props); // or deepFreeze()
    },
    componentWillUpdate: function(newProps) {
        Object.freeze(newProps); // or deepFreeze()
    }
};

(As mentioned above -- I don't have to deep freeze the object again if I deep froze it already, because those properties that were deeply frozen are still the same references.)

Sorry this is so long.

I can of course keep using my FreezePropsMixin. (Of course it's kind of a pain to apply a mixin to every component, but not that bad.) If others share this issue I can think of two ways a solution could be built into the library:

• When passed-in-props are cloned (e.g. in the places mentioned above), check whether those passed-in-props were frozen or not, and if they were, freeze the cloned object as well.
• Some kind of 'strict' mode like @slorber suggests above, which would freeze the component's this.props whether or not they were passed in as frozen.

[slorber]

@davidtheclark So to make it short what I understand of your point is that freezing an object and injecting it as props does throw an error when you try to modify the object itself inside the component, but it does not prevent from modifying the object reference in the props object as the props is always a new unfrozen object

I mean you can't do this.props.object.property = "value" but you can still do this.props.object = {property: "value"}

Yes it would be nice if React let use automatically froze the properties.

Maybe you can build your own component factory, like I do in my framework Atom-React (http://stackoverflow.com/questions/25791034/om-but-in-javascript), to automatically include some mixins everywhere:
https://github.com/stample/atom-react/blob/master/src/atomReact.js#L166
However this is a problem with JSX as it is not able to infer the component displayName anymore

[gaearon]

I'm probably missing the point.

Why freeze this.props at all? I get why you want to freeze deeper objects (you might have references to them elsewhere), but since this.props is built by React by copying props you passed, the only way they could be mutated is if you write something like this.props.something = ... inside the component itself, or pass this.props to some external method.

I doubt the first use case is worth protecting against because it's really hard to write something like this by accident. The second use case, maybe more so.

What do you think? Am I missing something?

[davidtheclark]

I'll try to be more concise and clear.

@slorber I think we're on the same page. If this.props were truly frozen, you couldn't say this.props.anything = ... without throwing an error. I'll consider the suggestion of a custom factory --- good idea.

@gaearon Here are some reasons I'd want to freeze this.props: To take full advantage of the unidirectional flow React allows for, I don't want anyone working on the code to be mutating this.props. Most of those properties come top-down from data stores and I definitely do not want to inadvertently mutate the contents of those stores by reference. (I also want each component farther down the chain to reflect exactly what's in the store at all times.)

Both of the possibilities you raise for mutating this.props (this.props.something = ... or passing this.props to another function) are real concerns. I think the first is worth protecting against because it's always worth automatically enforcing a standard that you want to uphold, if you can, rather than just trusting that everybody writing or reviewing code will manually catch the flaw. (Seems to me that that's the reason Object.freeze() exists at all: so you can enforce the way you want an object to be used --- right?) The second is a pretty common source of bugs.

I can think of three ways to pass props: 1. regular JSX attributes passed one at a time, 2. full object passed as JSX spread attributes, 3. full object passed to a factory.

2 and 3 could be addressed if React were to check whether the object you passed in is frozen, and if so freeze this.props, correspondingly. (But you would have to make sure you passed frozen objects every time if you want frozen props every time.) All three could be addressed by a mixin like the above that freezes props at the component-level (but you would have to make sure you included it every time); or a React-strict-mode that freezes all props everywhere for you.

(I think none of those changes are too significant --- if the React-strict-mode were opt-in it wouldn't force anybody not being strict already to change the way they use React.)

[sophiebits]

After my #2540, React will warn you automatically if you mutate this.props.

[davidtheclark]

Very nice.

[cappslock]

Just want to voice more support for this behavior. It was surprising to me to find that the isRequired prop validator in particular just throws a console warning. It makes it difficult to test, for one thing.

[slorber]

@cappslock if I remember my tests correctly the 0.14 version will be better regarding props validation. Not sure but I think it throw errors, or at least log error instead of warning

[slorber]

Hey,

This is an old thread but as we recently moved react warnings from console.warn to console.error, I was wondering if it was possible to trigger a callback everytime React detects a problem.

I understand you may not want to throw on bad proptypes as it could lead to different app behaviors in dev/prod mode, but it would be really nice if we had the hability to plug our own code when something bad happens.

Let me explain: sometimes the developers do not watch the console and do not notice react errors are logged. If I could plug a alert("there's a bug, open your console and fix it!"); we would ship much less of this kind of bugs. Also I would activate this fail-fast alerting in production for only a subset of the users (like the company members).

Is this possible?

[c089]

@slorber in the meantime we are using a little hack in our unit test setup to fail tests when they cause warnings:

const consoleError = console.error;

console.error = function (firstMessage, ...rest) {
    if (firstMessage.startsWith('Warning:')) {
        throw new Error(`Unexpected React Warning: ${firstMessage}`);
    }

    return consoleError(firstMessage, ...rest);
};

process.once('unhandledRejection', (error) => {
    console.error(`Unhandled rejection: ${error.stack}`);
    process.exit(1);
});

[slorber]

@c089 I've used something similar in the past but I don't like to proxy console.error as if we do this it messes up with the stacktrace that is shown in chome console.

[jimfb]

@slorber You should take a look at #5306 and #5462

[caesarsol]

I like the proposals in this thread.

Is there a way to freeze all the props in some way, in the meantime?

[gaearon]

I’m interested in adding this to Create React App in the meantime: facebook/create-react-app#783.
Feedback is welcome.

[gaearon]

I think it’s pretty unlikely we’ll crash your app on a failing prop, but you can do this yourself by overriding console.error. Note that this makes your app less predictable. Since prop validation only happens in development, this means your app will fail in development in different ways than in production, potentially leading to unreproducible error reports from production.

That said we do “fail fast” on crashes now with the new error handling behavior in React 16. Instead of keeping the tree around in a broken state, we unmount it, but let you opt into graceful handling with error boundaries.

So, in a way, I think we did add fail-fast mode to React. (And you can achieve the same for warnings by overriding console.error if you wish so, with caveats above.)

[franciscop-invast]

I am doing this right now, and it seems to work pretty sweet:

describe("...", () => {
  beforeAll(() => {
    jest.spyOn(console, "error").mockImplementation(error => {
      throw error;
    });
  });

  afterAll(() => {
    console.error.mockRestore();
  });

  // ...
});

It will, only for testing, convert every console.error(error) into a throw error.