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