Skip to content
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
Open

Provide alternative to mixin #8

mikemintz opened this issue Aug 23, 2015 · 10 comments

Comments

@mikemintz
Copy link
Owner

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
Copy link

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
Copy link
Owner Author

@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
Copy link
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
Copy link
Owner Author

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
Copy link

mtsewrs commented Apr 25, 2016

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

@mikemintz
Copy link
Owner Author

@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
Copy link
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
Copy link
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
Copy link
Contributor

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

@krunaldodiya
Copy link

krunaldodiya 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
Projects
None yet
Development

No branches or pull requests

6 participants