React static methods are lost with the higher order component

[jordaaash]

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]

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]

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]

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]

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

[acdlite]

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

[jordaaash]

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]

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

[jordaaash]

Haha yeah. No good.

[jordaaash]

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]

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]

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]

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]

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]

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]

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]

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]

@jordansexton Can this be closed?

[jordaaash]

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]

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

[bamorim]

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]

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

[giladv]

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]

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]

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]

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