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

React static methods are lost with the higher order component #173

Open
jordaaash opened this issue May 4, 2015 · 25 comments
Open

React static methods are lost with the higher order component #173

jordaaash opened this issue May 4, 2015 · 25 comments

Comments

@jordaaash
Copy link

This example supposes react-router's willTransitionTo is being used, but the behavior is true for any static methods.

var React   = require('react');
var connect = require('flummox/connect');

var SomeRoute = React.createClass({
    statics: {
        willTransitionTo: function (transition) {
            transition.abort();
        }
    },
    render:  function () {
        return <div>{ this.props.someProp }</div>
    }
});

module.exports = connect(SomeRoute, {
    someStore: function (store) {
        return {
            someProp: store.getSomeProp()
        };
    }
});

willTransitionTo will never be called on SomeRoute because it's not copied over to the ConnectedComponent returned by connect.

This significantly affects server rendered components. Another example:

// client

var React   = require('react');
var Promise = require('bluebird');
var connect = require('flummox/connect');

var SomeRoute = React.createClass({
    statics: {
        willRender: function () {
            return Promise.delay(3000);
        }
    }
    // etc.
});

// server

router.run(function (Root, state) {
    var flux = new Flux;
    return Promise.all(
        state.routes.map(function (route) {
            if (route.handler.willRender != null) {
                return route.handler.willRender(flux, state);
            }
        }).filter(Boolean)
});

willRender will never be called because it doesn't exist on the handler.

@acdlite
Copy link
Owner

acdlite commented May 4, 2015

I'm not sure if copying over statics would be the best solution. (Though I'm curious how other people are handling this with HoCs.) For now you can manually attach static functions to the component:

let MyComponent = React.createClass({...});
MyComponent = connectToStores(MyComponent, {...});
MyComponent.willTransitionTo = function({...});
module.exports = MyComponent;

@jordaaash
Copy link
Author

The component then has to know it's going to be wrapped by the HoC which seems too messy for my taste. If we think about the HoC like a decorator, then we need to be able to treat the wrapped component as we would the component itself.

While it's not necessarily an issue that, say, instanceof SomeComponent fails, I would expect to be able to call static methods on the decorated component. I currently work around this issue like this:

// some_page.jsx
var SomePage = React.createClass({
    // etc
});

module.exports = connect(SomePage, {
    // etc
});

// some_route.jsx
var SomePage  = require('./some_page.jsx');
var SomeRoute = React.createClass({
    statics: {
        willRender:         function (flux, state) {
            return flux.getActions('someActions').fetch(state.params.someParam);
        }
        render: function () {
            return (<SomePage { ...this.props }/>);
        }
    }
});

Basically, separating routing/server rendering concerns from the HoC completely by actually creating another HoC to wrap it. But this means I'm creating a dummy component just for that purpose for every route that needs static methods.

@jordaaash
Copy link
Author

Also, if we use ES6, static methods are automatically inheritable and visible inside the class declaration where they are expected to be, and MyComponent.willTransitionTo is not. Having to declare static methods after the HoC wrapping because they get lost doesn't seem ideal, to me anyway.

@acdlite
Copy link
Owner

acdlite commented May 4, 2015

No, it's not ideal, but neither is copying over all statics automatically, IMO. Need to think of a better solution.

@acdlite
Copy link
Owner

acdlite commented May 4, 2015

Maybe by passing in some sort of whitelist to the HoC?

@jordaaash
Copy link
Author

Yeah, that's a possibility. But I don't necessarily think we need to copy over all static properties/methods. If we assume that React components are being passed to connect, statics declared in the statics object are known to be desired to be accessed somewhere. Is there a way to introspect that and just copy those over, effectively using that as the whitelist?

@acdlite
Copy link
Owner

acdlite commented May 4, 2015

Perhaps (I don't know off the top of my head) but what about ES6 classes?

@jordaaash
Copy link
Author

Haha yeah. No good.

@jordaaash
Copy link
Author

Hmm. So, what is bad about copying them all over?

And does the HoC currently take any other arguments besides a store string, array of store strings, or hash of store getters?

@acdlite
Copy link
Owner

acdlite commented May 4, 2015

Not currently, but the API will change in the next version to be more flexible, e.g. to support action injection.

Copying them all over feels bad because it's essentially a form of inheritance (a mixin), which higher-order components are designed to avoid. But maybe I'm wrong.

@jordaaash
Copy link
Author

Yeah. It's essentially a form of inheritance, but in the absence of a usable "method missing" construct in JS, isn't it sort of impossible to have transparent decorators anyway? I guess I also think about this from the common use case in Flummox, which (I think?) is to connect() something once and just export the result of that, so that a component provisions its own store dependencies.

So while I agree with you in the academic sense, it seems like it fails the practical test to have to whitelist the methods you just declared, unless there are some you only care about internal to the file or component. I admittedly don't know much about the use cases for static methods in React outside of routing and rendering, so I have a hard time thinking of why it might matter.

I assume based on my own usage that the common use case for connect is to do it inside some_component.jsx, but if we posit a different use case for the theoretical whitelist, it becomes more clear that it increases coupling in your app to have to declare the statics:

var SomeComponent = connect(require('./some_component.jsx'), {
    someStore: function (store) {
        // etc.
    }
}, { // second arg is an options hash with a statics whitelist
    statics: ['willRender', 'willTransitionTo']
});

Now the parent is doing the requiring and connecting, and now it must know about the static methods of the child. This doesn't seem good at all.

@felipeleusin
Copy link

I've been messing with this for a while and i'm between two solutions when using HoC. The first one is create individual handler components that just implement the routerWillRun and the other is creating an single routerWillRun helper HoC and using it. Honestly I still don't know which we're going to end up using. The routerWillRun HoC looks like this

function routerWillRun(Component, func) {
  return class RouterWillRun extends React.Component {
    static routerWillRun(state, flux) {
      return func(state,flux);
    }
    render() {
      return <Component {...this.props} />;
    }
  };
}

and we call it like

export default routerWillRun(UserPage, function(state, flux) {
    return flux.getActions('someActions').fetchData(state.params.id);
})

@jordaaash
Copy link
Author

Hmm, okay. Since it's still a static method and would be lost to connect, does this mean you first call connect on the component (if it needs it)?

So it's essentially:

var UserPage   = React.createClass({ /*...*/ });
var Connected  = connect(UserPage, function () { /*...*/ ));
module.exports = routerWillRun(Connected, function () { /*...*/ }));

I guess this doesn't seem that bad, since it would be trivial to have an HoC that either copies additional static methods from the class, or declares them as in your example.

However, I think it's still surprising if a HoC isn't transparent with respect to the wrapped component's API, and the static methods are part of its API. That said, all HoC implementations I've seen are like this one and just copy ...this.props across and discard statics, so maybe it's not surprising to others.

@gaearon
Copy link

gaearon commented May 9, 2015

Proxying or copying seems worse to me than what we have to do now. It's more magic where we want less. If we do this for methods, we'll need to ask ourselves why aren't we doing it for properties too. For example, some library might want to read something from a static property instead, and we're back to where we started.

Adding a special whitelist seems broken too, because every HOC would do this in a different way, and some wouldn't do it at all, and it'd look pretty crazy.

If you want to put your statics on the exported stuff, you have two options:

  • make another wrapping HOC (seems wasteful, although more “pure”);
  • augment the HOC you got (I think it's perfectly fine in this case).
function statics(a) {
  return b => Object.assign(b, a)
}

// ...

import React from 'react';
import connect from 'flummox/connect';
import statics from './statics';

@statics({
  willTransitionTo(transition) {
    transition.abort();
  }
})
@connect({
  someStore: (store) => ({
    someProp: store.getSomeProp()
  })
})
export default class SomeRoute{
  render() {
    return <div>{this.props.someProp}</div>
  }
}

The nice thing about the decorator syntax is that you can still export default your class.

@felipeleusin
Copy link

Ugh.... I really dislike those decorators.... reading https://github.com/reactjs/react-future/blob/master/01%20-%20Core/02%20-%20Mixins.js it seems clear where react is heading, and it would be something ok.
Until them i'm prone to keeping the chainned HoC

@gaearon
Copy link

gaearon commented May 10, 2015

reading https://github.com/reactjs/react-future/blob/master/01%20-%20Core/02%20-%20Mixins.js it seems clear where react is heading

I would not say so:

https://twitter.com/sebmarkbage/status/571389309586051072

I'll focus on making composition easier so we can get rid of mixins.

Decorators are just sugar. You don't need to use them. My point being

import React from 'react';
import connect from 'flummox/connect';

class SomeRoute{
  render() {
    return <div>{this.props.someProp}</div>
  }
}

SomeRoute = connect({
  someStore: (store) => ({
    someProp: store.getSomeProp()
  })
})(SomeRoute);

Object.assign(SomeRoute, {
  willTransitionTo(transition) {
    transition.abort();
  }
});

export default SomeRoute;

the right way. i.e. instead of chaining, just define methods on whatever you export. Decorators help you achieve a nice syntax for that, but you can write it vanilla if you like.

@johanneslumpe
Copy link
Collaborator

@jordansexton Can this be closed?

@jordaaash
Copy link
Author

It's up to you. I know some maintainers prefer to close "stale" issues, but I don't see this as resolved at all. Personally, I'd as soon leave it open until there's some blessed method for dealing with this problem. So far using decorators as @gaearon suggested could be a good option, but decorators aren't even ES6, and there's little consensus on their use/desirability.

Static methods with react-router are both common internally with willTransitionTo/From, and the suggested way to call async routeWillRender-type methods isomorphically (at least under its current API, not sure about the 1.0/next branch that isn't documented yet).

@johanneslumpe
Copy link
Collaborator

Ok, then let's keep this open for now :)

@bamorim
Copy link

bamorim commented Jun 3, 2015

The approach I've used was defining another "connect" method that keeps my wanted static properties. It may seem a little bit of magic, but worked.

In util/connect.js:

import connectToStores from "flummox/connect";

export default function connect(Component,...args){                                                                                                                                            
  let { routerWillRun } = Component;
  let Connected = connectToStores(Component,...args);
  Object.assign(Connected, { routerWillRun });
  return Connected;
}

And in components/MyComponent.js:

import connectToStores from "../util/connect";

class MyComponent extends React.Component {
//...
  static async routerWillRun(){
    console.log("YEY");
  }
//...
}

export default connectToStores(MyComponent, ["stores"]);

What you guys think?

@gaearon
Copy link

gaearon commented Jun 4, 2015

Yes, I'm in favor of any userland solutions like this one.

@giladv
Copy link

giladv commented Jan 31, 2016

for me, the simplest solution is

@connect(..)
@someotherbrakingstuff...
export default class MyComp extends Component{
...
}

MyComp.routerWillRun = function(){

}

this solves the issue but is certainly not as good as "static myStatic = .."

@Pomax
Copy link

Pomax commented Feb 2, 2016

I've found the simplest solution to be "don't pretend they live on the HOC", and instead have the HOC grant access to the underlying component:

var React = require('react');
var Wrapped = function(Component) {
  return react.createClass({
    statics: {
      getClass: function() {
        return Component;
      }
    },
    getInstance: function() {
      varinstance = this.refs.instance;
      // we could be wrapping some other HOC. Turtles all the way down.
      if (typeof instance.getInstance === "function") {
        return instance.getInstance();
      } 
      // unless we run out of turtles.
      return instance;
    }
    render: function() {
      return <Component {...this.props} ref='instance'/>;
    }
  });
};
module.exports = Wrapped;

We can then, if we really do need to rely on statics, do:

var Thing = require('./hoc/wrapped/component.jsx');
Thing.getClass().runThisStaticFunction(...);

Although in a lot of cases, if you rely on HOC, the idea that your static functions need to live "on your component" becomes a bit muddled: the very nature of the static functions is that they run without any instance context, so other than "for looks", there is no programming reason to keep them defined in the class definition. You're usually far better off actually adding them into a utility library and simply straight-up using that directly. As static functions, there is nothing in the React class itself that they can rely on.

var Wrapped = require('hoc1');
var Chicken= require('hoc2');
var Burrito= require('hoc3');

var Thing = React.createClass({
  statics: require('./seprate-statics-file.jsx'),
  render: function() { ... }
});

module.exports = Wrapped(Chicken(Burrito(Thing)));

and then as part of a secondary static-utils lib (used for, say, static site generation or unit testing),

module.exports = {
  Thing:  require('./seprate-statics-file.jsx'),
  OtherThing:  require('./other-statics-file.jsx'),
  ...
}

Modifying the static functions is (if statics are done right) rare, but even if you need to, change it in a single location and all accessors still resolve correctly.

@Venryx
Copy link

Venryx commented Jan 10, 2017

I have a feeling this approach might be frowned upon, but the solution I'm currently using is the following:

Util.js

export function GrabInnerClass(grabFunc) {
    return targetClass=>grabFunc(targetClass);
}

MyComp.js

var MyComp_;

@connect(..)
@someotherbreakingstuff...
@GrabInnerClass(c=>MyComp_ = c) // just make sure this is the deepest decorator
export default class MyComp extends Component {
    static MyStaticMethod() {
    }
    
    MyInstanceMethod() {
        MyComp_.MyStaticMethod(); // call on MyComp_ instead of MyComp
    }
}

@zwhitchcox
Copy link

Hey, so, I was looking at this for an answer in my own product and found this solution, thought you guys might want to see it https://github.com/mridgway/hoist-non-react-statics

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