Warn if props obj passed into createElement / cloneElement inherits from anything other than Object

[richardscarrott]

#3435

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[richardscarrott]

It's failing linting because of the __proto__ references, I could alternatively use Object.getPrototypeOf(config) with a check to see if it exists?

[zpao]

@spicyj Do we still do some cheating with the props object in a React-internal component? I remember us doing something in the past…

[sophiebits]

@zpao TopLevelWrapper in ReactMount does but it doesn't use createElement.

[gaearon]

I think it’s fine to put this between /*eslint-disable no-proto*/ and /*eslint-enable no-proto*/.
It’s very intentional here and I fail to see the downside.

[gaearon]

Let’s avoid double negations. You want to specify the “normal” condition in the warning call. The normal condition is when the __proto__ ain’t there or it is Object.prototype. So let’s write it like

 warning(
  config.__proto__ == null || config.__proto__ === Object.prototype,

instead.

[gaearon]

This looks mostly good but let’s fix the linting and the couple of nits I left. Thanks for contributing!

[facebook-github-bot]

@richardscarrott updated the pull request.

[richardscarrott]

@gaearon I've made those changes, let me know if there's anything else that needs updating.

[sethkinast]

It's my understanding that the intent of warnings in React are "things you really should fix," but my app uses a Redux store full of custom Model objects that are really just POJOs with a couple static properties (so I can infer what the type of the backing data-model is). Now my console is full of warnings, but I don't think that there's anything actionable. I don't try to get data from some object up the prototype chain. Is there anything I can do?

[sophiebits]

@sethkinast Can you give more details? React.createElement already does ignore the prototype chain so it is probably an error to use an object with a prototype. Moreover, React.createElement pulls off the "key" and "ref" properties so you'd also get strange behavior if those were to be present in the models you're using as props – and it's not clear to me what you would write if you did want to set a key or ref. We generally suggest putting your model one level lower if you want to pass it unchanged, so props would be like {model: ...} and you use this.props.model.

[sethkinast]

I have a Model class that is basically

class Model {
  constructor(initFrom) {
   process(this.constructor.schema, initFrom);
  }
}

Then, when I get data from various APIs and persist it to my store, I first use the data to create objects that extend from Model to process the data (coercing types, validating fields):

class PersonModel extends Model {
  static schema = {
    name: 'string',
    id: 'int',
    avatar: PersonAvatarModel // defined in the same way elsewhere
  }
  static modelType = "identity/person"
}
let spicyj = new PersonModel({ name: "spicyj", id: "123" });
spicyj.id // 123, not "123"
spicyj.avatar // an instance of PersonAvatarModel, has some props of its own

I use those models, which are really just plain objects that happen to have something along their proto chain, and render components that often correspond 1:1 to the model.

render() {
  return <SomePersonComponent {...spicyj} />;
}

I could refactor all my components to instead do <SomePersonComponent model={spicyj} /> if that's the agreed-on pattern, but then I must use PropTypes.shape() everywhere.

[gaearon]

I would recommend putting toJS() on the base Model class that would be like

toJS() {
  return Object.assign({}, this);
}

Then you can

render() {
  return <SomePersonComponent {...spicyj.toJS()} />;
}

which doesn’t seem like a lot of hassle.

[gaearon]

The slightly confusing part about this is that any methods on it wouldn’t get bound, but seems like this is already an issue with the way you’re doing it. Maybe toJS() could specifically pick non-function properties, if it helps.

[sethkinast]

Yeah, these objects have no non-static methods, so that's not a concern.

On Thu, May 26, 2016, 12:37 PM Dan Abramov notifications@github.com wrote:

The slightly confusing part about this is that any methods on it wouldn’t
get bound, but seems like this is already an issue with the way you’re
doing it. Maybe toJS() could specifically pick non-function properties,
if it helps.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6134 (comment)

[sophiebits]

I guess <SomePersonComponent {...spicyj} /> should probably pass {...spicyj} (not just spicyj) to React.createElement.

[gaearon]

This wouldn’t let us warn at all though.

[sophiebits]

It would for people calling React.createElement directly. I guess I think of ... as losing the prototype so that wouldn't surprise me – but I don't know exactly what #3435 was originally trying to fix.

[sethkinast]

That's actually exactly how I thought of {... as well-- that you're just spreading out the object's props, and the object itself is subsumed.

[trxcllnt]

@spicyj @gaearon I realize this may sound absurd, but I have a use-case for props inheriting from something other than Object.

I'm writing a library for integrating redux and falcor. Lists are represented as maps in Falcor, which enables us to retrieve partial lists, or to have non-index keys in lists, e.g. falcor.get('genres[5..10].genreName', 'genres[5..10][0..10].movieName').

We can't create Arrays for the output JSON because non-indexer keys don't survive serialization, but the downside is we give up the Array prototype methods for lists.

But I've recently discovered that if I Object.create(Array.prototype) the output branch nodes instead, non-indexer enumerable own-keys will survive serialization, and I can also use the Array prototype methods (which work as long as a length value is present).

I've tested this works in practice, so now my only problem is React complaining that my props aren't strictly Objects. I don't expect this.props to inherit from Array, so it'd be nice if I could short-circuit or otherwise disable this warning.

[sophiebits]

I don't think I fully understand how you are using Falcor and how that relates to props, but basically: React.createElement looks at .key and .ref on the "props" second argument and the actual props that the component gets are copied off of that – it's not the same object. So passing another object with a non-Object prototype isn't actually useful because the component won't receive the prototype, and it obscures the fact that .key and .ref are treated specially. I think it's a good thing that React warns you in this case.

If you want to pass an object with prototype methods, just pass it as any key on props – like props.data or props.genres – and then React won't touch it.

[trxcllnt]

@spicyj I don't think I've made myself clear. I don't mind that the Array prototype methods won't make it onto this.props. I actually prefer they don't. But I would like to ignore the warning that I passed an Object to props which doesn't inherit directly from Object.

const rootProps = Object.create(Array.prototype);
React.render(
    <RootComponent {...rootProps}/> // <-- complains that rootProps isn't an Object
);

If you're not familiar, Falcor builds a JSON tree that you can pass to React. The JSON tree will contain maps and maps-that-represent-lists. Since Falcor can't distinguish between maps and Arrays, every branch in the tree will be created via Object.create(Array.prototype).

falcorModel.get(
        'genres[0..1].genreName',
        'genres[0..1][0..1].movieName'
    )
    .subscribe(({ json }) => {
        /*
        json === {
            genres: {
                0: {
                    genreName: 'Action Movies',
                    0: { movieName: 'Die Hard' },
                    1: { movieName: 'Lethal Weapon' }
                },
                1: {
                    genreName: 'Romantic Comedies',
                    0: { movieName: 'The Notebook' },
                    1: { movieName: 'How to Lose A Guy in Ten Days' }
                }
            }
        }
        */
        React.render(<RootComponent {...json}/>); // <-- complains :(
    });

[sethkinast]

This is the same sort of thing as the issue I mentioned earlier in this
issue. Both could be resolved by passing the splatted object instead of the
object itself.

@spicyj I looked at the babel plugin that handles this transpilation but it
was not immediately evident to me how to properly handle the case. I can
spend some more time digging in.

On Mon, Aug 8, 2016, 6:17 PM Paul Taylor notifications@github.com wrote:

@spicyj https://github.com/spicyj I don't think I've made myself clear.
I'm don't mind that the Array prototype methods make it onto this.props.
I actually prefer they don't. But I would like to ignore the warning that I
passed an Object to props which doesn't inherit directly from Object.

const rootProps = Object.create(Array.prototype);React.render(
<RootComponent {...rootProps}/> // <-- complains that rootProps isn't an Object
);

If you're not familiar, Falcor builds a JSON tree that you can pass to
React. The JSON tree will contain maps and maps-that-represent-lists. Since
Falcor can't distinguish between maps and Arrays, every branch in the tree
will be created via Object.create(Array.prototype).

falcorModel.get(
'genres[0..1].genreName',
'genres[0..1][0..1].movieName'
)
.subscribe(({ json }) => {
/* json === { genres: { 0: { genreName: 'Action Movies', 0: { movieName: 'Die Hard' }, 1: { movieName: 'Lethal Weapon' } }, 1: { genreName: 'Romantic Comedies', 0: { movieName: 'The Notebook' }, 1: { movieName: 'How to Lose A Guy in Ten Days' } } } } */
React.render(<RootComponent {...json}/>);
});

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6134 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABVa7Ntz4Okyj5jI16OS3xaRIS9IU3yfks5qd9UHgaJpZM4HkinZ
.

[trxcllnt]

@sethkinast This is the same sort of thing as the issue I mentioned earlier in this issue. Both could be resolved by passing the splatted object instead of the object itself.

Agreed. I initially assumed the JSX Object-spread syntax behaved the same as ES6 Object-spread syntax, which doesn't complain about merging in this case 😁

[philiptzou]

This warning will be raised (incorrectly?) if React.createElement received object created by another v8 vm, especially when doing server-side rendering. Here's the reason:

const evaluate = require('eval');

const O2 = evaluate("module.exports = Object;", "example.js", {}, true);
console.log(O2 === Object); // output false
console.log(O2.prototype === Object.prototype); // output false

Also see the issue reported on node-eval.