Feeback wanted: Rewrite with ideas from @acdlite

[gaearon]

So @acdlite got me a birthday present (yay it's my birthday today :-): this gist.

It shows how much easier Redux can be implemented if we embrace single-atomness and instead of hiding it, make it explicit.

I changed a few things from his implementation:

• Instead of pushing atom down the context, I push subscribe() to avoid shouldComponentUpdate problems;
• Instead of relying on setState, maintain this.atom in Dispatcher so setState is called both on the root and the children at the same time and the atom is never stale in the action creators;
• Add a convenience combineStores higher-order store (lol) that converts an object map of Stores into a Store function that combines them.

I combined our two examples to better show how this works together.
I like it.

Would love to hear your thoughts.

@acdlite @ooflorent @threepointone @vslinko

TODO

• Initial implementation of Store composition
• Performance work
• Extract dispatcher for non-React use, make it part of public API
• Figure out the API for dispatching actions (remove actions from props #45, Ideal redux API #48)
• Support isomorphic apps @acdlite
• Update README

[gaearon]

composeStores will turn a stores exported object into a normal Store with state, action => state signature that would combine them. It's a good shortcut, and you may replace it with a custom mechanism like a single Immutable Map if you want to. cc @fisherwebdev

[vslinko]

Happy Birthday!
Right now I don't understand how to make this isomorphic — how to prepare atom before render?

[gaearon]

@vslinko

Thanks!

@acdlite, counting on your help with this :-). I don't have experience with isomorphic apps but Andrew built Flummox so he should know.

It's a good point. I want to merge this after it supports in some form all of the use cases currently PR'd by @vslinko: #41 #39 #33 #32.

[ooflorent]

Lodash's mapValue would be more readable.

[gaearon]

Yeah, might use that. I kinda like the rawness though (I didn't realize I could use reduce like this).
I don't have strong opinion here. (Note it's also used in combineStores)

[gaearon]

@ooflorent I addressed both your comments

[gaearon]

So, the things still bugging me:

• Lack of support for isomorphic (progressive, universal, whatever) apps — hoping for help from @acdlite
• Should we call it atom? I don't want to scare off people used to Flux terminology who feel things like Om, NuclearJS or Omniscient are above their head.

[gaearon]

@ooflorent

I pushed some perf optimizations: b70739a.
It's possible to optimize further (in fact even more!) if we add a select prop to Injector:

// Before
export default class CounterApp {
  render() {
    return (
      <Injector actions={CounterActions}>
        {({ state: { counter }, actions }) =>
          <Counter counter={counter} {...actions} />
        }
      </Injector>
    );
  }
}


// After
function select(state) {
  return state.counter;
}

export default class CounterApp {
  render() {
    return (
      <Injector actions={CounterActions}
                select={select}>
        {({ state, actions }) =>
          <Counter counter={state} {...actions} />
        }
      </Injector>
    );
  }
}

Note that this also frees you from destructuring inside Injector child function.

The select prop is just a function that picks the relevant part of the tree. The results of previous and next select runs would be compared shallowly, and if they are equal, rendering would be skipped. This is kinda like cursors, but without the word “cursor” and string keypath arrays.

What do you say? If you're fine, I'd be happy to merge a PR in this branch implementing select.

[gaearon]

Note: I renamed atom to state. No functional lingo for now in the public API. :-)

[ghost]

Happy birthday!

You must be truly coming from future or different universe...
After renaming atom to state, its easier to understand. At least I think I've got it ;-)

[gaearon]

@ooflorent

I added a select prop, this should solve your perf issues: 18b0695

This lets you reach into the global state as deep as you want and return an object that will be shallowly compared on each change. If the object is shallowly equal, shouldComponentUpdate bails out. This is actually a potentially more performant solution than what we had in Redux before.

By default, select is just an identity function.

[ooflorent]

What about:

select: oneOfType([ string, func ]).isRequired

And then:

import get from 'lodash/object/get'
import isFunction from 'lodash/lang/isFunction'

const slice = isFunction(this.props.select)
  ? this.props.select(atom)
  : get(atom, this.props.select)

[gaearon]

I thought about that.. But then, people are going to want arrays of string keys for keyPaths and we're back to cursors :-). I think functions (or lambdas) work great here.

[gaearon]

So here's what I got API-wise:

Decorator version

import React from 'react';
import { inject } from 'redux';
import Counter from '../components/Counter';
import * as CounterActions from '../actions/CounterActions';

@inject({
  actions: CounterActions,
  select: state => ({
    counter: state.counter
  })
})
export default class CounterApp {
  render() {
    return (
      <Counter {...this.props} />
    );
  }
}

Component version

import React from 'react';
import { Injector } from 'redux';
import AddTodo from '../components/AddTodo';
import TodoList from '../components/TodoList';
import * as TodoActions from '../actions/TodoActions';

export default class TodoApp {
  render() {
    return (
      <Injector actions={TodoActions}
                select={state => state.todos}>
        {this.renderChild}
      </Injector>
    );
  }

  renderChild({ state, actions }) {
    return (
      <div>
        <AddTodo {...actions} />
        <TodoList todos={state} {...actions} />
      </div>
    );
  }
}

Is the state/action merging too implicit? Can we make it better now that we have select?
@threepointone

[gaearon]

Since the actions references don't change, I wonder if it's too much to put them in select too? Then select can become the only parameter. (See also #45)

import React from 'react';
import { inject } from 'redux';
import Counter from '../components/Counter';
import * as CounterActions from '../actions/CounterActions';

@inject((state, wrap) => ({
  counter: state.counter,
  ...wrap(CounterActions)
}))

[vslinko]

👍 for select

[gaearon]

I don't quite like this wrap thing. It would need to keep some kind of memoized cache of bound action creators, or otherwise it would be really hard to implement a sensible shouldComponentUpdate for the components.

Maybe do it like this?

import React from 'react';
import { inject, perform } from 'redux';
import Counter from '../components/Counter';
import * as CounterActions from '../actions/CounterActions';

@inject(state => ({
  counter: state.counter
}))
export default class CounterApp {
  const { dispatch, counter } = this.props;
  render() {
    return (
      <Counter counter={this.props.counter}
               {...perform(dispatch, CounterActions)} />
    );
  }
}

import React from 'react';
import { Injector, perform } from 'redux';
import AddTodo from '../components/AddTodo';
import TodoList from '../components/TodoList';
import * as TodoActions from '../actions/TodoActions';

export default class TodoApp {
  render() {
    return (
      <Injector select={state => state.todos}>
        {this.renderChild}
      </Injector>
    );
  }

  renderChild({ state, dispatch }) {
    const actions = perform(dispatch, TodoActions);
    return (
      <div>
        <AddTodo {...actions} />
        <TodoList todos={state} {...actions} />
      </div>
    );
  }
}

This would also fix #45.

The difference with the previous suggestion is that, because perform is called inside the container component, this let us have the efficient shouldComponentUpdate without any memoizing at the Injector level. You are also free to pass dispatch down as is and call action creators from the leaf components.

perform(dispatch, actionCreator) would work both with a single function and with an object map of functions, and would wrap the action creator.

[vslinko]

Since you are extracted Dispatcher then redux is isomorphic now.
Are you sure Dispatcher should be a class? I prefer createDispatcher

[vslinko]

Is this sufficient for (de)hydration?

Yes, thank you.

[acdlite]

What's the advantage of emitting changes manually like this instead of using setState() on the root + context in the provider? Is it to avoid a context bug?

[gaearon]

Yes. It works around the shouldComponentUpdate context bug. Also context is not meant for passing often changing data due to other performance implications (I believe @sebmarkbage warned me about this). By using a manual system, we're only using the context for creating a wormhole, but allow fine-grained subscriptions at any level of the tree.

[gaearon]

There are also other problems with using just setState. For example, the state passed to action creator might be stale because there are pending operations that have not yet been rendered. We are able to sidestep this problem completely by maintaining the atom manually.

[acdlite]

This looks really great. I dig the select API — that's the perfect verb, I think, to describe what that does.

The only thing that's stopping this from being 100% isomorphic is when you do createDispatcher() outside the lifecycle of the root component.

[vslinko]

Can you help me here? What do I need to do to address your concerns from #48 (comment)?

perform should pass itself as first argument into action instead of dispatch:

 perform(actionCreator, ...args) {
    const action = actionCreator(...args);

    return typeof action === 'function'
      ? action(this.perform, this.atom)
      : this.dispatch(action);
  }

Usage example:

function syncAction() {
  return {
    type: 'A'
  }
}

function asyncAction() {
  return perform => {
    perform(syncAction())
  }
}

function anotherAsyncAction() {
  return perform => {
    perform(asyncAction())
  }
}

dispatcher.perform(anotherAsyncAction())

[vslinko]

Missed that perform shouldn't call actionCreator itself, it takes an action:

 perform(action, ...args) {
    return typeof action === 'function'
      ? action(this.perform, this.atom)
      : this.dispatch(action);
  }

=>

export default function bindActions(actionCreators, dispatcher) {
  return mapValues(actionCreators, actionCreator =>
    (...args) => dispatcher.perform(actionCreator(...args))
  );
}

[gaearon]

@vslinko I need to go now, can you PR your suggested changes to action creator calls relative to this branch?

[vslinko]

@gaearon I'll try

[gaearon]

The only thing that's stopping this from being 100% isomorphic is when you do createDispatcher() outside the lifecycle of the root component.

Can you elaborate on this? What do I need to change?

[gaearon]

Anything else left re: isomoprhic apps? @acdlite @vslinko

[vslinko]

Anything else left re: isomoprhic apps?

No, It's ready.

[acdlite]

As is, the isomorphism story is already way better than any other Flux library, but if you want out-of-the-box isomorphism with no extra set-up — the same way hot reloading "just works" — then you'd have to move createDispatcher() inside the component lifecycle of the root component and interact with it via props.

[gaearon]

Unfortunately I don't have expertise here so can't comment. Can you give me an idea of how that would be used on the server side? I know how (de)hydrate API can be used, but I'm not sure what you're suggesting. (on server, there is no mounting so no prop lifecycle, is there?)

[acdlite]

It's not a huge deal, but currently on the server the user would need to make certain they do createDispatcher() on every request.

app.get("/", function(req, res) {
  const dispatcher = createDispatcher(store);

  res.send(
    React.renderToString(<Provider dispatcher={dispatcher} />)
  );
});

Not bad at all, but someone could accidentally making the mistake of calling createDispatcher() outside of the request handler.

I think it would be nice to have this option

app.get("/", function(req, res) {
  res.send(
    React.renderToString(<Provider store={store} />)
  );
});

and call createDispatcher() internally. You'll still probably want to provide the first option so you can easily access dispatcher methods for dehydration etc., but I like how this option "just works."

[gaearon]

How would you solve this API-wise while still providing the (de)hydrate API?
I'd prefer Provider to accept a single thing (either dispatcher or store) for API consistency.

[vslinko]

👎 for <Provider store={store} />

[acdlite]

For extra APIs like dehydrate, then yes, passing the dispatcher is the way to go. I personally don't think supporting both APIs is that bad (it's only two props total!), and I like the simplicity of just passing a store, but it's not a huge deal to me.

[acdlite]

Also, my perspective is admittedly a bit skewed because in my gist, literally the only stateful part is the atom, whereas Redux also keeps track of subscriptions (for good reasons) and needs to provide a direct interface for dehydration, etc.

[gaearon]

This is out as 0.8.0.
Huge thanks to everyone, especially @acdlite.