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

Add combineReducers #19

Open
stasm opened this issue Sep 18, 2017 · 13 comments · May be fixed by #22
Open

Add combineReducers #19

stasm opened this issue Sep 18, 2017 · 13 comments · May be fixed by #22

Comments

@stasm
Copy link
Owner

stasm commented Sep 18, 2017

When working on medium-sized apps, it's helpful to enforce the separation of concerns by writing separate reducers for different parts of the state tree. An example of this is routing which needs its own state and logic.

I'd like to add an optional combineReducers function to innerself. Perhaps in a new module: innerself/combine?

@bsouthga created his own implementation in innerself-hg and I used this one in A moment lost in time.

I wonder if we should also add selectors to connect. innerself actually used to have selectors when I first wrote it. I then removed them to keep things simple.

Selectors are functions which select a part of the state before it's passed on to the connected component. Something like:

const ConnectedFoo = connect(state => state.foo)(FooComponent);

In Redux selectors are super-useful: they help Redux decide if the connected component needs to update. In innerself everything re-renders at all times, so the main benefit would be decoupling the shape of the state tree from the API of individual components.

Even though this looks nice and makes the code self-documenting, I expect the most common scenario to not need selectors. We'd then force the connect(state => state)(…) boilerplate onto everyone or have a special-case for the undefined selector: connect()(…). This in turn would be different from how Redux treats the undefined selector: it ignores the store completely. Perhaps that's OK. If you want to ignore state, you simply don't connect your component to it in innerself.

For now in my code I went for the following pattern whenever I needed selectors:

const ConnectedFoo = connect((state, ...args) => FooComponent(state.foo, ...args));

This pattern could be easily abstracted to a helper function, for instance:

function select(selector, component) {
    return (state, ...args) => component(selector(state), ...args);
}

And then:

const ConnectedFoo = connect(select(state => state.foo, FooComponent))                                                                                                                                                                                      

This helper could also live in the optional module alongside combineReducers. I'm not particularly fond of this API, though.

@stasm
Copy link
Owner Author

stasm commented Sep 18, 2017

cc @bsouthga, @flynnham, @bcruddy, @wrick17

@stasm
Copy link
Owner Author

stasm commented Sep 18, 2017

Another idea: connect could take a selector as an optional second argument:

const ConnectedFoo = connect(FooComponent, state => state.foo);

@f1yn
Copy link
Contributor

f1yn commented Sep 18, 2017

While I can't say I'm experienced with Redux (yet), I can say that having the selector as the second argument would be beneficial, since it could denote a simple state => state selector as the default (as you already pointed out).

@stasm, to be be clear, the selectors would allow specific renders to happen only when the correct state changes, correct? If so then yeah, I am on-board with this sort of change.

@wrick17
Copy link

wrick17 commented Sep 18, 2017

I don't really selectors are going to make much of a difference performance wise. The whole block is re-rendered anyways. However it helps in code sanity, so I think it should be fine as an optional parameter.

Also I was actually thinking about combineReducers this weekend. it would be a great (and necessary) addition. Of course as an additional module.

@stasm
Copy link
Owner Author

stasm commented Sep 18, 2017

@stasm, to be be clear, the selectors would allow specific renders to happen only when the correct state changes, correct? If so then yeah, I am on-board with this sort of change.

@wrick17 is right — this won't affect performance. Innerself just re-renders the entire attach-ed component every time its output changes.

And I also agree with the point about code sanity. That's what I meant by "self-documenting". Selectors help express in code which part of the state tree the component needs without hard-coding the shape of the tree into the component itself.

@wrick17
Copy link

wrick17 commented Sep 18, 2017

@stasm Makes sense. Putting it as an optional parameter sounds good to me. People can choose to write less code or could go for more readable code for bigger projects.

@bsouthga
Copy link
Contributor

bsouthga commented Sep 18, 2017

Adding a combineReducers function would be nice, definitely something that will usually be useful.

As for selectors, one option might be to adopt/suggest a props pattern similar to react for component arguments, this might make composing components easier (in terms of dealing with an object vs a variable number of arguments), as well as allow for selecting multiple properties like mapStateToProps:

import html from 'innerself';
import { connect } from '../store';

const ConnectedFoo = connect(
  state => ({ bar: state.innermost.prop.bar }),
  ({ bar }) => html`
    <div>
        check out my bar: ${bar}
    </div>
  `
);

(the above would be achievable with your selector proposal in general, just thought it could be a nice pattern to suggest, build around)

@bsouthga
Copy link
Contributor

I also like the idea of mirroring react-redux's HOC pattern for connect by returning a function which takes a component.

const ConnectedFoo = connect(state => ({ bar: state.inner.bar }))(Foo)

@stasm
Copy link
Owner Author

stasm commented Sep 18, 2017

Yeah, I like that too. Redux's connect is a configurable decorator which is a really nice design pattern.

Originally, I didn't want to enforce any pattern wrt. props vs. arguments passed to components because I didn't want to make the design more complex without a good reason. That reason might have just revealed itself :)

@stasm
Copy link
Owner Author

stasm commented Sep 18, 2017

What I like about the variable args patterns is that you can then do things like ${tasks.map(ActiveTask)}.

@bcruddy
Copy link
Contributor

bcruddy commented Sep 20, 2017

Optional parameters or returning a function that accepts a component both seem fine to me with a slight preference towards variable args.

My main concern around all of this is the potential for innerself to get away from what it really excels at - being ultra lightweight with minimal setup to put together proofs-of-concept or small personal projects. With its limitations around render speed (e.g. always re-rendering everything) it seems like pushing developers to break apart state in reducers encourages building large, complex applications that will suffer massively in rendering performance. The rendering issue has been solved - if you need high performance rendering react, angular, and vue are much better choices than innerself.

All of that being said - with a router on the horizon, multiple reducers do seem to be the logical next step and the const ConnectedFoo = connect(state => ({ bar: state.inner.bar }))(Foo) pattern is already well established in the community.

@stasm
Copy link
Owner Author

stasm commented Sep 20, 2017

The rendering issue has been solved - if you need high performance rendering react, angular, and vue are much better choices than innerself.

I completely agree. And I share your concerns—I wouldn't want innerself to get too advanced precisely because I believe that its simplicity communicates its purpose.

@bsouthga has a good point about how this discussion also touches the good practice around the API of components. If the reason to introduce selectors is to improve code sanity and separate the operation of connecting a component from defining it, we shouldn't taint the signature of components with the connected state. The current solution of passing the state as the first argument doesn't scale well: it tight-couples the component's definition with the fact that it is supposed to be connected.

React/Redux have a good solution for this, as @bsouthga already pointed out: the pattern of passing a props object as the first argument. I've come to think that innerself should encourage this pattern too. Dumb components don't have to follow it, but the ones which might become connected components should. To ensure components can be re-used, I think it makes sense to establish a good practice of using props.

I have a WIP branch locally which I'll push later tonight for your review.

@stasm stasm linked a pull request Sep 20, 2017 that will close this issue
@stasm
Copy link
Owner Author

stasm commented Sep 20, 2017

Alright, I submitted #22 and would love to hear your thoughts about it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants