New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide alternative to mixin #8

Open
mikemintz opened this Issue Aug 23, 2015 · 10 comments

Comments

Projects
None yet
6 participants
@mikemintz
Owner

mikemintz commented Aug 23, 2015

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

This comment has been minimized.

motebotic commented Nov 18, 2015

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

This comment has been minimized.

Owner

mikemintz commented Nov 18, 2015

@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

This comment has been minimized.

Contributor

birkir commented Mar 16, 2016

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

This comment has been minimized.

Owner

mikemintz commented Mar 19, 2016

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.

@mattiasewers

This comment has been minimized.

mattiasewers commented Apr 25, 2016

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

@mikemintz

This comment has been minimized.

Owner

mikemintz commented Apr 25, 2016

@birkir does the decorator functionality work for you now that #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

This comment has been minimized.

Contributor

harlantwood commented Jul 2, 2016

@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 added a commit to harlantwood/react-rethinkdb that referenced this issue Jul 2, 2016

@harlantwood

This comment has been minimized.

Contributor

harlantwood commented Jul 2, 2016

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

harlantwood@6ee0914

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

This comment has been minimized.

Contributor

harlantwood commented Jul 3, 2016

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

@kunaldodiya

This comment has been minimized.

kunaldodiya commented Dec 22, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment