uber-archive / r-dom

React DOM wrapper
MIT License
263 stars 18 forks source link

Add property inheritance to conveniently pass parent properties down to child components. #21

Closed cain-wang closed 9 years ago

cain-wang commented 9 years ago

Conveniently pass down parent properties to child components. This small feature saves many boilerplate code where we'd need to manually map the parent properties to child properties or assign blocks that copy over the parent properties.

Previously, people have to do:

var Parent = React.createClass({
  render: function render() {
    r(Child, {
      id: this.props.id,
      name: this.props.name,
      valid: this.props.valid
      // the list could be long for complicated components and tedious for wrapper components!
    });
  }
});
var Parent = React.createClass({
  render: function render() {
    r(Child, {
      inheritProps: this.props // Inherits all properties from parent.
    });
  }
});

var Parent = React.createClass({
  render: function render() {
    r(Child, {
      inheritProps: {
        props: this.props,
        includes: ['id', 'name'] // Inherits properties named ['id', 'name'].
      }
    });
  }
});

var Parent = React.createClass({
  render: function render() {
    r(Child, {
      inheritProps: {
        props: this.props,
        excludes: ['id', 'name'] // Inherits all properties from parent except ['id', 'name'].
      }
    });
  }
});
mishabosin commented 9 years ago

This is cool, thanks for building this!

I'm a concerned about the current implementation though. If I didn't know that there is an onChange in this.props the following code could take a while to debug:

var Parent = React.createClass({
  render: function render() {
    r(Child, {
      inheritProps: this.props, // Inherits all properties from parent.
      onChange: this.onMyChange // will be overwritten by this.props.onChange!
    });
  }
});

I think we should change it so that explicit properties will always overwrite inherited props, or at least warn the user that there is a collision.

As long as it doesn't impact performance, I think this is an awesome idea!

cain-wang commented 9 years ago

That's a good point. I agree, explicit property should always has a higher priority, and we probably should throw out a warning if an overriding occurs.

rtsao commented 9 years ago

I actually prefer the following idiom for this functionality, which also eliminates any ambiguity of what takes priority (the latter always takes priority).

var pick = require('lodash.pick');

var Parent = React.createClass({
  render: function render() {
    r(Child, {
      color: 'blue',
      ...pick(this.props, 'color', 'name')
    });
  }
});

vs

var pick = require('lodash.pick');

var Parent = React.createClass({
  render: function render() {
    r(Child, {
      ...pick(this.props, 'color', 'name'),
      color: 'blue'
    });
  }
});

Similarly for omit:

var omit = require('lodash.omit');

var Parent = React.createClass({
  render: function render() {
    r(Child, {
      color: 'blue',
      ...omit(this.props, 'id', 'name')
    });
  }
});

vs

var omit = require('lodash.omit');

var Parent = React.createClass({
  render: function render() {
    r(Child, {
      ...omit(this.props, 'id', 'name'),
      color: 'blue'
    });
  }
});

If you can't use spread operators, you can just use Object.assign, lodash.defaults, etc. to achieve the same effect. For example:

var pick = require('lodash.pick');
var assign = require('lodash.assign');

var Parent = React.createClass({
  render: function render() {
    r(Child, assign({
        color: blue
      }, pick(this.props, 'id', 'name'))
    );
  }
});

I don't think any of these forms are significantly burdensome so I feel that inheritProps is probably an unnecessary abstraction. I'm also bit worried that building it into r-dom might encourage unnecessary passing of props, which can be abused in place of better component architecture.

cain-wang commented 9 years ago

Yeah, unfortunately without JSX, using spread might be problematic for now .. We are already using Object.assign() to pass down properties right now and the syntax is still cumbersome.

Since react doesn't have component inheritance in favor of component composition, sometimes it gets in the way of creating reusable components. For instance, I created a uber-button, which is a regular button with uber styles, then in my project, I need a project specific button, which should inherits all properties from uber-button, it is very hard to do with current react design.

That's why I'm thinking inheriting component properties might be the first step towards that.

rtsao commented 9 years ago

These style props never change?

Just a thought: what if you put the style props into mixin with a getDefaultProps method containing your styles?

rtsao commented 9 years ago

There's obviously the burden of requiring omit/pick and defaults/extend if you don't use lodash, but I personally find both syntaxes more or less equally cumbersome, with the former being more explicit regrading how the inheritance actually works:

var omit = require('lodash.omit');
var defaults = require('lodash.defaults');

var Parent = React.createClass({
  render: function render() {
    r(Child,
      defaults({
        foo: 'bar'
      }, omit(this.props, 'id', 'name'))
    );
  }
});
var Parent = React.createClass({
  render: function render() {
    r(Child, {
      foo: 'bar',
      inheritProps: {
        props: this.props,
        excludes: ['id', 'name']
      }
    });
  }
});

You could do it like this which is even closer in syntax:

var excludes = require('lodash.omit');
var inherit = require('lodash.defaults');

var Parent = React.createClass({
  render: function render() {
    r(Child, inherit({
      foo: 'bar'
      }, excludes(this.props, ['id', 'name']))
    );
  }
});

That said, I'd argue for using babelify and just using spread operators which I think is the best of both worlds and aligns with what is encouraged when using JSX.

cain-wang commented 9 years ago

If we can use spread assignment, that'll be the best :) Using babelify steps onto the fine line whether we are writing javascript that'll be executed directly in the browser.

But recently I discovered react used to have similar construct in early releases, and they depreciated that. Probably for future release they'll try to switch to React context.