mikemintz / react-rethinkdb

Render realtime RethinkDB results in React
MIT License
870 stars 48 forks source link

Provide alternative to mixin #8

Open mikemintz opened 9 years ago

mikemintz commented 9 years ago

Mixins don't work with React components written as ES6 classes.

Let's provide something that will work like a superclass, higher order component, or ES7 decorator.

motebotic commented 8 years ago

Did you ever solve this issue?

I am still too new to solve this myself, but I am going to try none the less using https://github.com/timbur/react-mixin-decorator

What are your thoughts?

mikemintz commented 8 years ago

@motebotic That seems like a reasonable solution. Is that something you can use easily in your own project without requiring changes in react-rethinkdb? If you have to go through any hoops to get it working, it'd be great if you could write it up here so we can have it documented for other users.

I haven't yet decided how to provide an official alternative to mixin. One of the big downsides to me with react-mixin-decorator's approach is that it creates a higher-order component. While that works fine in most cases, it violates the principle of least surprise, because the decorated component actually becomes the HOC in practice. So if you decorate a component like UserList, and then render that somewhere as <UserList ref="mylist" />, then this.refs.mylist will return the HOC instead of the UserList instance.

I don't know the full ramifications of this, but I'm hoping best practices will emerge (or maybe they already have?)

birkir commented 8 years ago

I've successfully used the mixins as decorators by using the babel-plugin-transform-decorators-legacy plugin.

The helper:

import { BaseMixin } from 'react-rethinkdb';

const Rethinkable = sessionGetter => component => {
  const proto = BaseMixin.call(component.prototype, sessionGetter);
  const unmount = proto.componentWillUnmount;

  proto.componentDidMount = function () {
    this._isMounted = true;
  };

  proto.componentWillUnmount = function () {
    this._isMounted = false;
    unmount.call(this);
  };

  for (let key in proto) {
    const _proto = component.prototype[key];
    component.prototype[key] = function (...args) {
      proto[key].call(this, ...args);
      if (_proto) {
        _proto.call(this, ...args);
      }
    }
  }
};

export default Rethinkable;

Usage:

import React, { Component } from 'react';
import { r, QueryRequest } from 'react-rethinkdb';
import Rethinkable from '../helpers/rethinkable';

const session = component => component.props[name];

@Rethinkable(session)
class App extends Component {

  observe (props, state) {
    return {
      turtles: new QueryRequest({
        query: r.table('turtles'),
        changes: true
      })
    };
  }

  render () {
    return (
      <ul>
        {this.data.turtles.value().map(turtle => (
          <li key={turtle.id}>{turtle.name}</li>
        ))}
      </ul>
    );
  }
}

Only thing needed modification in the codebase is https://github.com/mikemintz/react-rethinkdb/blob/master/src/util.js#L21 and needs to support checking for component._isMounted gracefully like this:

export const updateComponent = component => {
  // Check for document because of this bug:
  // https://github.com/facebook/react/issues/3620
  if ((component._isMounted || component.isMounted()) && typeof document !== 'undefined') {
    component.forceUpdate();
  }
};

It doesn't brake the old method and should't be too much overhead. https://facebook.github.io/react/blog/2015/12/16/ismounted-antipattern.html

mikemintz commented 8 years ago

That seems pretty cool! Maybe since it seems like isMounted() might get deprecated anyway, we should first patch the mixin so it sets _isMounted in componentDidMount and componentWillUnmount, and then modify updateComponent to use only _isMounted kind of like you suggest.

Then with that in place, we can add your decorator as a mixin-alternative.

mtsewrs commented 8 years ago

@mikemintz Any plans or news regarding implementing something like this anytime soon?

mikemintz commented 8 years ago

@birkir does the decorator functionality work for you now that https://github.com/mikemintz/react-rethinkdb/pull/31 is merged? Maybe we could add your decorator code as well if you're ok with that, so that other people can use the decorator easily.

harlantwood commented 8 years ago

@birkir this seems like a nice solution.

I can't quite get it to work though; I get the error:

Uncaught Error: Mixin in does not have Session

from this line:

proto[key].call(this, ...args);

Also, I notice that my component.props[name] is null at the time it is evaluated...

Can you perhaps push a full working example to a branch of your fork? Thanks : )

harlantwood commented 8 years ago

@birkir, I duplicated the "tutorial" example in my fork, and applied your code it as I understand it:

https://github.com/harlantwood/react-rethinkdb/commit/6ee091475e0865eaaaeaf9c00bf7a21da718c87c

I get the same error in the browser console: Uncaught Error: Mixin in does not have Session.

Am I doing something wrong? Or does the react-rethink codebase need further modifications?

harlantwood commented 8 years ago

BTW, I tried up updating all dependencies (including react) to their latest versions; behavior was unchanged.

krunaldodiya commented 7 years ago

can someone give me working example for ES6 of this repo ???

as my project is completely built in react ES6 and i am unable to use this with..

if any one has any working example of react-rethinkdb in ES6 pls, provide me