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

Higher order component or container component instead of mixin #44

Closed
ksmth opened this issue May 8, 2015 · 17 comments
Closed

Higher order component or container component instead of mixin #44

ksmth opened this issue May 8, 2015 · 17 comments
Assignees

Comments

@ksmth
Copy link

ksmth commented May 8, 2015

As React is moving away from mixins, it might make sense to start talking about alternatives. Maybe something along the lines of:

function nuclearComponent(Component, dataBindings, reactor) {
  return class NuclearComponent extends React.Component {
    constructor() {
      this.state = getState(reactor, dataBindings)
    }

    componentDidMount() {
      var component = this
      component.__unwatchFns = []
      each(dataBindings, function(getter, key) {
        var unwatchFn = reactor.observe(getter, function(val) {
          var newState = {};
          newState[key] = val;
          component.setState(newState)
        })

        component.__unwatchFns.push(unwatchFn)
      })
    }

    componentWillUnmount() {
      while (this.__unwatchFns.length) {
        this.__unwatchFns.shift()()
      }
    }

    render () {
        return <Component {...this.props} {...this.state} />;
    }
  }
}

Which you could then use like:

// ES7
@nuclearComponent(dataBindings, reactor)
class MyComponent extend React.Component {
    // ...
}
// ES6
class MyComponent extend React.Component {
    // ...
}

MyComponent = nuclearComponent(MyComponent, dataBindings, reactor)
@mindjuice
Copy link

Turns out there was some misunderstanding on this. Mixins are not going anywhere (at least not soon). The React Router guys reverted their code back to mixins.

https://github.com/rackt/react-router/blob/master/UPGRADE_GUIDE.md

I prefer to avoid ES6 classes for as long as possible (hopefully forever). I would prefer a ''this-less" style for React.

@ksmth
Copy link
Author

ksmth commented May 9, 2015

@mindjuice could you please elaborate on a this-less style? You can easily use React.createClass() instead of class NuclearComponent extends React.Component {}, as that's just an implementation detail.

How is

var MyComponent = React.createClass({
    // ...
})
MyComponent = nuclearComponent(MyComponent, dataBindings, reactor)

less 'this-less' than

var MyComponent = React.createClass({
    mixins : [ flux.ReactMixin ]
    // ...
})

I think it's nicer, if you don't force others to use a certain feature if it's not necessary. To reduce code duplication, you could even use the mixin internally:

function nuclearComponent(Component, dataBindings, reactor) {
  return React.createClass({
    mixins : [ flux.ReactMixin ],
    getDataBindings() { return dataBindings; }
    render () { return <Component {...this.props} {...this.state} /> }
  }
})

@mindjuice
Copy link

Programming JS without this basically involves using functions to create "objects" out of closures, as is often done in other functional languages like Scheme. You get clean, simple access to your data by name only without worrying about what object this points to at any given time (since there isn't a this). The tradeoff is "objects" usually take more space.

Anyway, I generally avoid this, classes and inheritance in my non-React code.

Of course, with React I don't get to avoid this, but they made it nicer by ensuring that this is properly bound for all functions and event handlers. You still have issues when using things like _.map() and the like where you resort to the var that = this; ugliness.

My comment was just wishful thinking that React would move to a "thisless" style. It's something that they have thought about based on comments I've see before. Also, there is this "declarative" experiment to create a React component in the react-future repo.

I don't follow your comment about forcing others to use a certain feature. Which feature is being forced onto others?

@ksmth
Copy link
Author

ksmth commented May 9, 2015

By only providing a mixin, as a consumer of Nuclear I'm forced to write the components in a certain way. I understand, that this is sometimes necessary when choosing your tools / libraries.

You can use the decorators under all circumstances, while mixins are only working with React.createClass().

@mindjuice
Copy link

I see. Yes, it would be nice if it worked regardless of which way you chose to structure your objects.

Decorators would require ES6 though, so perhaps it could provide both mixins and decorators.

@ksmth
Copy link
Author

ksmth commented May 9, 2015

It'd require ES7 even, if you really want to use the @decorator syntax. But that's only syntactic sugar, which is nice. The functions themselves are backwards compatible, even with ES5. You just call the function with the object you want to decorate as the first argument.

MyComponent = decorator(MyComponent)

I'd definitely be willing to contribute a PR if there's interest in this.

With the approach outlined above it'd result in that the requested bindings are no longer available on this.state, but on this.props of the decorated component and I'm not certain on whether that's a good thing or not. On the one hand, it changes the API and decorated components would work differently to the components using the mixin. On the other hand, it also makes the decorated component more reusable and decoupled from Nuclear, which might be a good thing.

@jordangarcia
Copy link
Contributor

I will definitely investigate this more, as its my goal to have first class support for use of with React out of the box.

However I imagine there are ways to use a Nuclear Reactor to create some sort of NuclearComponent class with the current library, much like @ksmth shows in the example code.

The following statement is from the React v0.13.0 Beta 1 blog post

Therefore, we will keep working with the larger JS community to create a standard for mixins. We will also start designing a new compositional API that will help make common tasks easier to do without mixins. E.g. first-class subscriptions to any kind of Flux store.

If React adopts a good solution support a more compositional paradigm I believe that would suit Nuclear's functional style.

Until then it seems fine for people to create their own NuclearComponent class that has the same functionality of the mixin.

@ksmth if you are interested in PRing an example of using this style of NuclearComponent class I would be happy to merge and link from the README as the prescribed way of using NuclearJS with React ES6 classes.

@colindresj
Copy link
Contributor

I'm with @ksmth on this one. While mixins are still the idomatic way of sharing code across React components, it's pretty clear that they're not ideal. It seems to me that the React team is waiting to see what patterns emerge in the community, and I think Nuclear should be making an effort towards contributing to that.

I'm not suggesting replacing flux.ReactMixin, but instead proving a second/alternative baked-in manner for sharing store binding logic.

I see a few benefits to using a higher order function instead of a mixin:

1 - Leans on composition over inheritance
2 - Removes state from presentational components
3 - Allows data fetching components to make sure that all the data required to render a presentational component exists before that component's render function is called (no more if (!this.state.someVal) return null)
4 - Makes it much clearer when a particular lifestyle hook is invoked on a component, because they're all only ever declared within it's class declaration

It seems to me that Relay Components are pushing for this sort of pattern, and I think we can substitute graphql with Nuclear getters to achieve something similar.

I like a slightly modified API to what @ksmth first wrote. The difference being, I think you can safely rely on the factory living off of the reactor, and I wouldn't pass state down into child components.

class MyComponent extends React.Component {
  render() {
    return <div>{this.props.someValue}</div>
  }
}

export default flux.Component(MyComponent, {
  someValue: SomeFluxModule.Getters.someValue
})

A possible, more JSX derived, alternative might look like this:

import {NuclearComponent} from 'nuclear-js'
import SomeFluxModule from './some-flux-module'

const dataBindings = {
  someValue: SomeFluxModule.Getters.someValue
}

class MyComponent extends React.Component {
  render() {
    return (
      <NuclearComponent dataBindings={dataBindings}>
        <MyChildComponent />
      </NuclearComponent>
    )
  }
}

class MyChildComponent extends React.Component {
  render() {
    return <div>{this.props.someValue}</div>
  }
}

I strongly encourage more debate on introducing a non-mixin-based way to get Nuclear store data into a component.

@rattrayalex
Copy link

fwiw I find a standardized NuclearComponent to be pretty interesting, for many of the reasons listed above. Mixin's certainly shouldn't go anywhere though.

Hopefully once a few members of the community have built & used their own for a bit we can get a PR. I may try this myself.

@jtremback
Copy link

One problem with JSX components is they will only work on a particular platform. We'd have to have separate components for DOM, iOS, Android, etc etc. Not to mention other frameworks!

@colindresj
Copy link
Contributor

Don't think that's necessarily true. JSX isn't a templating language like we're used to (handlebars, lodash, etc), because it isn't meant to build up HTML; it's a declarative syntax for building up virtual DOM. That reason is exactly why it can be used to create native iOS UI elements, android elements, and even arbitrary trees representing anything you'd like.

Getting a component to work against multiple platforms would/should be an implementation detail in my opinion.

@clessg
Copy link

clessg commented Jun 6, 2015

I definitely agree with having a HoC/decorator/wrapper.

@cef62
Copy link

cef62 commented Jun 12, 2015

Having an alternative to Mixins would be great!

@Sinewyk
Copy link
Contributor

Sinewyk commented Jul 22, 2015

https://github.com/jordangarcia/nuclear-js-react-addons should suit your needs.

Let us know what you think.

@colindresj
Copy link
Contributor

Looks awesome @Sinewyk @jordangarcia. Really like that it supports decorators too

@walkerrandolphsmith
Copy link

Glad I came across this after it was solved :).

@bhamodi
Copy link
Contributor

bhamodi commented Nov 1, 2015

Closing this as it can be found here: https://github.com/jordangarcia/nuclear-js-react-addons

@bhamodi bhamodi closed this as completed Nov 1, 2015
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