reduxjs / react-redux

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

Alternative API proposals #1

Closed gaearon closed 9 years ago

gaearon commented 9 years ago

This API is taken from Redux <Provider>, <Connector>, @provide, @connect. Right now I don't have examples in this repo but it matches what you've seen in Redux so far.

It's the best we've got now, but I think we can do better!

Common pain points:

Let's go wild here. Post your alternative API suggestions.

They should satisfy the following criteria:

taylorhakes commented 9 years ago

I don't like the <Redux>. I think it is confusing with v0.12 createRedux() (which wasn't a React component) and makes this component stand out too much.

I am a big fan of the named params on the connect. I think that is less confusing and allows the user to specify the arguments in any order. It also allows for adding to the API later.

I also agree that select doesn't quite fit. My experience with select is linq in C# https://msdn.microsoft.com/en-us/library/bb397927.aspx (Ahh Microsoft!) I like slice or pick, those are familiar from a lot of the JS util libraries. What about sliceFn or pickFn? I know we have docs and type annotations coming, but it makes it very clear what it needs to be just by seeing the param name.

Edit: typo

faassen commented 9 years ago

I like the general direction in which this is going -- it makes it possible to entirely keep any Redux related stuff out of plain React components, including the whole actions mechanism.

Getting props to tweak action creators is an important use case, I think, so it's important it's made easy. In hypermedia-based UIs you often pass URLs around as properties and then pass them into action creators Note that this is distinct from the parameterized action creator use case where you want to parameterize a whole bunch of related actions.

I'm not sure I fully understand the consequences of merge yet. Is the primary reason 'merge' is not the second argument (replacing the actions arg with a dispatch arg) for performance reasons or for usability reasons? If performance reasons, does this mean that any kind of parameterized action will degrade an application's performance because props vary more? Action tweaking in merge creates new functions, which could potentially cause props to be different. But should merge be called at all if the state and props haven't changed since the last time? Could merge be memoized?

merge is responsible for creating the entire prop object that goes into the underlying component. That makes it very flexible and I appreciate that, but I would like to explore approaches where the developer doesn't have to worry about all that. I'd like to support something like this:

function tweak(counters, actions, props) {
  const { counterId } = props;
  const { increment, decrement } = actions;
  return {
    counter: counters[counterId],
    increment: () => increment(counterId),
    decrement: () => decrement(counterId),
  };
}

You could write a higher order function that does this:

function createMerge(func) {
   return (state, actions, props) => {
        const tweaked = func(state, actions, props);
        return { ...props, ...tweaked };
  };
}

const merge = createMerge(tweak);

Are performance optimization opportunities lost this way?

From the readability perspective I have a slight preference for named parameters. But named parameters have the drawback that is supports some uses cases (like binding actions without state) I understand are less than optimal without signalling the developer (by requiring a null arg) that they are taking a suboptimal approach. Named parameters have the benefit of extensibility: it would make it easy to add a tweak parameter. But I think that's the kind of extensibility you don't want at this level. So I can see reasons for using normal arguments in this case.

sporto commented 9 years ago

I personally don't like binding actions creators at the top level and passing them, I cannot see how is that an anti-pattern. Your top component has to know too much about what the children need.

I prefer passing the dispatch function to my components, and have them call it directly by importing the action declaration themselves. In this way I can postpone the decision of what actions I need until I actually need to trigger one down the line. I can even pass the dispatch function to something else.

So please allow for this use case.

vslinko commented 9 years ago

:+1: for "Normal arguments"

jlongster commented 9 years ago

Hey all,

I have what's probably a dumb question. I've understood and followed Flux for a long time, but I've never actually built a large app with it, so I don't have as much experience with advanced use cases. For server-side rendering, do you ever actually fire actions on the server? I'm not sure why you would, it seems like action firing is only in response to some external event.

Server-rendering seems like one of the main reasons to not create global action creators that are already bound to your store. Although, another argument is probably that it's clearer to list the actions that a component depends on. But I'm wondering when the former matters, if you don't fire actions on the server? I get that listing actions per-component is clearer though.

skevy commented 9 years ago

@jlongster you definitely can fire actions on the server. For instance, what if you have an action that "gets" data. You may want to do that when bootstrapping a component for instance. And you'd want that to work on both the client and server.

gaearon commented 9 years ago

Hey @jlongster, thanks for coming by!

For server-side rendering, do you ever actually fire actions on the server? I'm not sure why you would, it seems like action firing is only in response to some external event.

You want to fire actions to prepare the stored state. This way you can render actual state before sending it to the client.

Server-rendering seems like one of the main reasons to not create global action creators that are already bound to your store.

Yes, this is the main reason why we don't bind right away.

gaearon commented 9 years ago

I prefer passing the dispatch function to my components, and have them call it directly by importing the action declaration themselves. In this way I can postpone the decision of what actions I need until I actually need to trigger one down the line. I can even pass the dispatch function to something else.

So please allow for this use case

This use case is allowed by the current proposal. You can definitely grab dispatch and pass it down. I guess it comes down to how reusable you want your components to be, and how often the same component do different actions in different contexts. It depends on the app.

jlongster commented 9 years ago

@skevy @gaearon Thanks! I'm rewriting my blog admin to use Redux and finally making time to think through this.

Currently, all my components can define a static method which can fetch data (performed when I resolve all the routes and stuff), and this data is automatically available. Other than data fetching, are there any other use cases? I'm trying to reconcile Relay with this, and I see some opportunities. With Relay that data fetching happens automatically for you. Since Relay isn't released yet, I don't know how you perform updates, but I think the general flux-style flow of actions works for that.

If I solve the data fetching part, are there any other reasons why I'd need redux on the backend?

EDIT: May just have to wait for Relay, and I could just follow the patterns here and use Redux for data fetching too. Still thinking through.

gaearon commented 9 years ago

We def want to integrate with Relay when it's out. Redux can then specialize in handling local state and complex local mutations.

guillaume86 commented 9 years ago

The bindActionsCreators and dispatch thing got me thinking a bit. I'm currently creating my store in a dedicated file and bind all my actions creators at the same place (the lazy method :) ).

Something like that:

store.js

const store = createStore(...);
export default store;
export const actions = bindActionCreators(..., store.dispatch);

and just reference this module to call actions in smart components (this part could use a decorator to pass arbitrary data into props to make it more explicit that this is a smart component):

MyComponent.js

import { actions } from './store';

class MyComponent {
  handleClick() {
    actions.doSomething();
  }

  ...
}

(Side note: I'm not the only one doing something like that, see https://github.com/jhewlett/react-reversi/blob/master/js/actions/gameActions.js)

That got me thinking, what would be the downside in removing <Provider/> and @provide and use the store instance directly in the @connect decorator like this:

MyComponent.js

import store, { actions } from './store';

@connect(store, state => ...)
class MyComponent {
  handleClick() {
    actions.doSomething();
  }

  ...
}

Maybe I'm missing something (any advantages in having the store in context?) but it seems more simple than the current API and easier to explain, it's actually one level "lower" than the current API, and the current API could be rebuilt on this in a separate lib.

It could also be simplified a bit by using a method to enrich the store with the react connect method:

store.js

const store = reactRedux.makeConnectable(createStore(...));
export default store;
export const actions = bindActionCreators(..., store.dispatch);

MyComponent.js

import store, { actions } from './store';

@store.connect(state => ...)
class MyComponent {
  handleClick() {
    actions.doSomething();
  }

  ...
}

Any thoughs on this? It's just an idea I don't use React/redux much except for a toy project so I'm not really a reference in this area but it never hurt to share ideas.

vslinko commented 9 years ago

export default store; is singletone. Singletones doesn't work for universal apps, because on server each client needs personal store.

guillaume86 commented 9 years ago

Ha ok I see the advantage of having a short lived store instance in the components context in that case.

quirinpa commented 9 years ago

Hey guys i love alternative API, so what format of it is available in redux-rc?

gaearon commented 9 years ago

@quirinpa

Redux 1.0 RC does not include React bindings. We separated them in this repository precisely because we want to iterate on them separately.

React-Redux 0.2.1 still has the old API. If you'd like to implement the API proposed here, you're welcome to send a PR! We can then publish it as 0.3.0.

quirinpa commented 9 years ago

@gaearon Thanks man but i think i still i don't have the knowledge to :P you have developed awesomeness, thanks!

ForbesLindesay commented 9 years ago

One small thought, but shouldn't:

connect(
  state => ({ state.counter }),
  dispatch => ({
    increment: dispatch(CounterActions.increment())
  })
)(Counter) 

be

connect(
  state => ({ state.counter }),
  dispatch => ({
    increment: (...args) => dispatch(CounterActions.increment(...args))
  })
)(Counter) 

Otherwise you would be firing the increment action immediately?

gaearon commented 9 years ago

@ForbesLindesay Right, my typo.

quirinpa commented 9 years ago

Just a substitute until Keats solution is merged :P

import React from 'react';
import { bindActionCreators } from 'redux';
import { Connector } from 'react-redux';
import { isEqual } from 'lodash';

export function connect(select, createActions = () => ({}), merge = (s, a, p) => ({...s, ...a, ...p})) {
    const subscribing = select ? true : false;

    return DecoratedComponent2 => class ConnectorDecorator2 {
        shouldComponentUpdate(nextProps) {
            return subscribing && !isEqual(this.props, nextProps);
        }
        render() {
            return (
                <Connector {...(select ? { select: state => select(state, this.props) } : {})}>
                    {stuff => {
                        const { dispatch, ...rest } = stuff;
                        const actions = (typeof createActions === 'function') ?
                            createActions(dispatch) :
                            bindActionCreators(createActions, dispatch);
                        return <DecoratedComponent2 {...merge(rest, actions, this.props)} />;
                    }}
                </Connector>
            );
        }
    };
}

Edit: I've made a few changes. It now supports the following;

// -------------- EXAMPLE --------------------

import { connect } from 'tools/redux';

@connect(
  null, // provide select or null
  CounterActions // provide actionCreators or function that binds to dispatch, for example:
     // dispatch => ({ incrementCounter: (...args) => dispatch(CounterActions.increment(...args))})
     // or bindActionCreators.bind(undefined, CounterActions)
)
export default class Counter { /* . . . */ }

@gaearon i'm probably doing something wrong and i'm not very confortable with git, so i'd rather hear from you before i send a PR... Cheers.

Edit: Now supports merge :)

Keats commented 9 years ago

Since we are removing Connector in #16, should we also remove Provider ? This way this package only exposes 2 functions that can be used as decorators or plain functions.

gaearon commented 9 years ago

@Keats Yeah we might. I haven't looked into it yet, want to get the Connector stuff sorted out first.

gaearon commented 9 years ago

@Keats To clarify, I don't propose anybody actually remove it before we discuss. There are many concerns: universal rendering, hot reloading, DevTools, etc, and we want to make sure we handle all these scenarios gracefully.

gaearon commented 9 years ago

I think we should remove provide(). It doesn't work for server rendering (store is different on each request). It also doesn't work for Redux DevTools because they need a separate provider. It locks you into bad patterns.

The only downside of <Provider> is this function-as-a-child gotcha. It will be solved in React 0.14, so I'm convinced that removing provide() is the way forward.

mmerickel commented 9 years ago

Late to the conversation but wanted to add an alternative I haven't seen discussed for an action api. One thing I've noticed when writing my first couple redux apps is that redux does a fantastic job of getting almost all of the coupling of my model out of my components. I wanted to continue this trend by removing any coupling to the exact actions creators I'm invoking. The idea was to bind the actions to the dispatcher at the same/similar time that I "bind" the store to the react component tree. In this way I didn't need to import and/or know exactly which action creator I was invoking. I simply use it by name just like I use the state.

@connect(
    (state, props) => ({
        todos: state.todos,
    }),
    (actions/*, dispatch? */) => ({
        addTodo: actions.addTodo,
    }),
)
class MyAppContainer extends React.Component {
    render() {
        const {addTodo, todos} = this.props;
        return (
            <div>
                <button click={addTodo}>Add Todo</button>
                <ul>{todos.map(::this.renderTodo)}</ul>
            </div>
        );
    }
}

const store = createStore(reducer);
const actions = {
    addTodo(name) {
        return {type: ADD_TODO, name};
    }
};

React.render(<Provider store={store} actions={actions}>{() => <MyAppContainer />}</Provider>, document.getElementById('app'));

This is nice because action creators tend to store most of the business logic in the app and they often need things like api utils, etc. This pre-binding is a perfect opportunity to configure your action creators with things like an apiUtils object and then that stuff doesn't need to be known about in the view components and your action creator also doesn't need to be coupled to a specific apiUtils singleton like I see in many examples.

To be clear the Provider would be responsible for binding all actions passed into it to the store. This could be separated into a StoreProvider and ActionProvider if desired, and do not need to be done at the same time but I think that may be too much. The Provider then just adds the store and actions context values allowing @connect to access them.

ghost commented 9 years ago

I second the idea @mmerickel describes above. I was actually going to suggest exactly the same thing since I have been binding the actionCreators at the top level and passing them down. I'd love if actions could be "selected" in the same way as slices of state in the connect decorator.

I've been staying away from using dispatch in my dumb components and instead just using pre-bound actionCreators from props, but I like that there is a very easy way in his sample above to get at the dispatcher. To me this supports both styles of working with actions, and because the api would be very similar to what is already understood for selecting state, it would reduce cognitive load required to start being productive with the react-redux bindings.

gaearon commented 9 years ago

@danmartinez101 @mmerickel

One of the reasons I don't want to do this is because code splitting will be harder. People are likely to keep wanting the same structure, and will do hacks to make it work with code splitting. The current proposal works with code splitting with no modifications.

I feel this is more opinionated than I'm willing to go. I'm happy to see this explored in alternative bindings though!

Everyone, thanks for the discussion, let's continue in #16.

mmerickel commented 9 years ago

@gaearon Can you clarify what you mean by code splitting? My assumption by exposing the dispatch to the action getter was that this would help if you didn't want to do <Provider actions={actions}>. Maybe exposing a binder instead of dispatch works better which seems quite similar to what your original proposals have been above.

gaearon commented 9 years ago

@mmerickel Code splitting = when your app's modules are loaded by demand. As result, not all actions are available from the beginning.

mmerickel commented 9 years ago

That's an issue with the single store as well isn't it? Presumably when you add reducers you can subscribe to that change and add actions as well, no?