reduxjs / react-redux

Official React bindings for Redux
https://react-redux.js.org
MIT License
23.38k stars 3.36k forks source link

connect() doing something to static methods in parent class? #393

Closed braco closed 8 years ago

braco commented 8 years ago

No idea why this is happening. Any clues?

class Bar {
  static renderNavigationBar(props) {
    return <CustomView />;
  }
}

class Foo1 extends Bar {}

The function of the static method is probably irrelevant, but it tells rn to use that custom view for the navigation bar: https://github.com/facebook/react-native/blob/master/Libraries/CustomComponents/Navigator/Navigator.js#L1120

This works as expected:

export default Foo1;

And without inheritance, this works as expected:

class Foo2 {
  static renderNavigationBar(props) {
    return <CustomView />;
  }
}
export default Foo2;

But with inheritance, this doesn't work – it's as if there was no static method on parent.

export default connect(...)(Foo1)

Is this a redux issue?

gaearon commented 8 years ago

Only “own” static methods are proxied for simplicity. This is intentional. Please don’t use inheritance with React. It’s never necessary with it. https://github.com/reactjs/react-redux/issues/383#issuecomment-220020842

braco commented 8 years ago

Thanks @gearon. How would that work with a static method on the parent container, though? static renderNavigationBar() still wouldn't get picked up with the technique you linked to:

class Foo1 extends Component {
  render() {
     // static renderNavigationBar method is on Bar
     <Bar>
          ...
     </Bar>
  }
}
export default Foo1;
gaearon commented 8 years ago

I don’t fully understand what you’re trying to do. Maybe if you could post a more complete example to StackOverflow, I could answer!

braco commented 8 years ago

For people searching:

Foo1 needs

static renderNavigationBar() {}

This method is reused across many classes. Inheriting from a parent class doesn't work with redux apparently.

I guess the following options would work

import {renderNavigationBar}  from ...;
class Foo1 extends Component {}
Foo1.renderNavigationBar = renderNavigationBar;
function enhanceComponent(component) { component.renderNavigationBar = ...; }
export default enhanceComponent(Foo1)

or use es7 decorators in the future?

import { Bar } from "./Bar";

@Bar
class Foo1 extends Component {}

Which is unfortunate for something this simple

gaearon commented 8 years ago

@braco I’d like to help you but I need you to help me first. Can you please publish a complete example showing why you want it to be a static method, and how it gets used? It is not obvious.

It’s not that something “doesn’t work in Redux”, it’s that the solution is likely very simple, but since I don’t understand what you’re building, I can’t help you.

gaearon commented 8 years ago

I understand now that the static method gets used by React Native Navigator. But I don’t understand why you want Foo1 to extend Bar, and what Foo2 is. A complete specific example would help.

danibrear commented 8 years ago

TL;DR: Don't extend anything besides default React.Component in your components if you plan on connecting them to Redux.

I ran across this issue this morning (or something similar). I'm using react-native-router-flux and wanted to add a custom navbar to a bunch of scenes. I decided to abstract out the static renderNavigationBar(props) to a separate class like this:

export default class ComponentWithNavBar extends Component {
    static renderNavigationBar(props) {
        return (<CustomNavBar />);
    }
}

and use that in all of my classes like this:

export default class Profile extends ComponentWithNavBar {
    // *component code here *
}
/* different file */
export default class Settings extends ComponentWithNavBar {
    // *component code here *
}
/* etc */

This works great until you attempt to connect the components to redux via connect(...) because it seems the connect method is returning a new Component or as you said:

Only “own” static methods are proxied for simplicity. This is intentional.

For anyone who cares how I solved this, I just included a standard

static renderNavigationBar(props) {
    return (<CustomNavBar {...props} />);
}

in each of my components. This serves to explicitly signify which components have a navigation bar as well as allow me to customize it a bit more than just extending a class. If this is not what this original ticket is about, I'm sorry for wasting so much of your time.

gaearon commented 8 years ago

TL;DR: Don't extend anything besides default React.Component in your components if you plan on connecting them to Redux.

This is generally a good rule in React. You shouldn’t use inheritance with it.

I decided to abstract out the static renderNavigationBar(props) to a separate class like this:

I would not recommend using inheritance for this. What if next time, you want to share some other code between other set of your classes? But now they inherit from a common parent, so you’d have to use some hack or a mixin system to copy those methods. At the end of the day there is no reason why it is shorter than just a function:

export default renderNavigationBar = ...
import renderNavigationBar from './renderNavigationBar'

class Component1 extends Component { }
Component1.renderNavigationBar = renderNavigationBar
import renderNavigationBar from './renderNavigationBar'

class Component2 extends Component { }
Component2.renderNavigationBar = renderNavigationBar

In React, inheritance is discouraged. Just don’t use it, you don’t need it. You can achieve exactly the same reusability with functions.