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

Chaining / combining ready-made connect functions? #522

Closed
clarabstract opened this issue Oct 13, 2016 · 19 comments
Closed

Chaining / combining ready-made connect functions? #522

clarabstract opened this issue Oct 13, 2016 · 19 comments

Comments

@clarabstract
Copy link

So, it turns out it is very handy and useful to just provide ready-made connect functions for some common use cases.

For example, our data fetching and caching subsystem provides it's own version of connect() that works exactly like the react-redux connect, except it can look at the output of mapStateToProps and if the values are instances of some special data-fetching-definition objects the same subsystem provides, it knows how to setup the initial data fetch and subsequent updates by passing through dispatch from mapDispatchToProps to mergeProps and so forth.

The problem is in order to do this we had to look at and re-implement connect's internal handling of mapStateToProps, mapDispatchToProps and mergeProps. This is brittle - we have to rely on keeping the re-implemented behavior in sync with you, even though this is a simple matter so far.

The problem actually gets worse the more of these ready-made connect functions we want to implement. There is a relatively simple connectForm I wish to add to easily hook-up forms to our form handling subsystem. It just maps a well known area of the redux state (looking up forms by name) to a form's field prop, and does a straight-forward map of form-related action creators to their respective event props. This should be trivial, but isn't, again, because we have to re-implement all that default functionality and carefully merge in the handful of things we want to do.

And worst of all the system breaks down entirely when trying to combine the two (e.g. a form that also needs to fetch its data) - it simply cannot be done.

The approach I have in mind is to pass in the connect function the custom-connects call internally as a 5th argument, but this feels like it's slowly getting out of hand.

Surely there can be a more well thought out composition mechanism provided by react-redux itself? I have some thoughts on how that could look but before I go down that road I'm wondering if there is existing work towards this, or if I'm missing a much more straight-forward way of solving these kinds of problems.

@jimbolla
Copy link
Contributor

Can you provide a more concrete example? Also, take a look at the codebase in the next branch of react-redux, which is rearchitected to be more customizable.

@clarabstract
Copy link
Author

Actually, soon after I wrote this it occurred to me composition can be made trivial by applying the HoC factory returned by all these connects to each-other.

E.g.

let ConnectedSomeFormComponent = connectForm(formName)(connectDataFetch(dataFetchMapper)(connect(regularMapStateToProps, regularMapDispatchToProps)(SomeFormComponent)));

Sure it looks a bit ugly but I think that should work OK? And there is no real need for the custom connects to support the same mapStateToProps etc arguments as the regular connect - one just needs to wrap a component with both.

@esamattis
Copy link
Contributor

I've used successfully flow and compose from lodash to combine HOCs.

import {flow} from "lodash/fp";

let ConnectedSomeFormComponent = flow(
  connectForm(formName),
  connectDataFetch(dataFetchMapper),
  connect(regularMapStateToProps, regularMapDispatchToProps)
)(SomeFormComponent);

@clarabstract
Copy link
Author

Re concrete example, here is the "trivial" implementation of connectForm that cannot be extended or combined:

import {connect} from 'react-redux';

import updateFieldValue from './updateFieldValue';
import commitFieldValue from './commitFieldValue';
import fieldWasValidated from './fieldWasValidated';

export default function connectForm(formName) {
  let mapStateToProps = (typeof formName === 'function'
    ? ({forms}, props) => {
      let formName = formName(props);
      let fieldData = formName in forms ? forms[formName] : {};
      return {formName, fieldData};
    }
    : ({forms}) => ({
      formName: formName,
      fieldData: formName in forms ? forms[formName] : {}
    })
  );
  return connect(
    mapStateToProps,
    {
      onUpdateFieldValue: updateFieldValue,
      onCommitFieldValue: commitFieldValue,
      onFieldWasValidated: fieldWasValidated,
    }
  );
}

// used like this
let ConnectedEditForm= connectForm(
  ({editObj}) => editObj ? `editForm.${editObj .id}` : `createNewObj`
)(EditForm);

... except, as I said above, it actually could be composed, like so:

let ConnectedEditForm= connectForm(
  ({editObj}) => editObj ? `editForm.${editObj .id}` : `createNewObj`
)(connect(regularMapStateToProps, ...etc)(EditForm));

@markerikson
Copy link
Contributor

FYI, you can use the compose utility to combine those together in a more readable form:

const combinedHOC = compose(
    connectForm(formName),
    connectDataFetch(dataMapper),
    connect(mapState, mapDispatch)
);

export default combinedHOC(MyComponent);

@clarabstract
Copy link
Author

Ahhhhhhhh - thank you @markerikson, it never occurred to me that it had applications for HoCs as well, not just middlewares :)

@markerikson
Copy link
Contributor

Amusingly, @epeli pointed it out at the exact same time I did, just right before your "concrete example" comment :)

@jimbolla
Copy link
Contributor

I was typing it too and then your answers came in. ;)

@clarabstract
Copy link
Author

Right, thank you all :)

Out of curiosity, given this seems to be a good and popular solution, what is the use case for the split up connect implementation in the next branch? Or is it just an internal clean-up?

@jimbolla
Copy link
Contributor

If you're curious, #407 and #416 contain the info dump for the refactorings.

@markerikson
Copy link
Contributor

See #407 and #416 for the gory details, but it's basically a complete reimplementation to improve performance and fix edge cases. Courtesy of @jimbolla .

@markerikson
Copy link
Contributor

Okay, people, we have GOT to stop simul-posting... :)

@esamattis
Copy link
Contributor

For me the most important thing is that it allows to write custom connect()s easily. Shameless self promotion: https://github.com/epeli/lean-redux

@j18ter
Copy link

j18ter commented Feb 15, 2018

Coming to this a bit late, but here is my question. I understand the compose approach, and this works in many cases, but what if I don't want to write three separate functions that return bits and pieces of the props. How would one combine these wrapping functions in a way that takes a single function to compute the props? Let me give a concrete example, say I want to combine react-redux's connect and react-meteor-data's withTracker wrapping functions, Using compose I would do it this way:

const combinedHOC = compose (
    withTracker(mapCollectionsToProps),
    connect(mapState, mapDispatch)
);

Not only do I need to write three separate functions mapCollectionsToProps, mapState, and mapDispatch, the mapCollectionsToProps function receives props, but not state as argument. This function may want to access state too. I guess a similar question is why does connect need two separate functions, one to map state and a separate function to map dispatch to props, instead of just a single function that receives both state and dispatch as arguments?

@markerikson
Copy link
Contributor

@j18ter : because some components may want to subscribe to the store state and not dispatch actions, or vice versa. So, by specifying those separately, we can ensure that a component that only wants to dispatch actions never has to subscribe to the store state updates, thus improving performance.

@j18ter
Copy link

j18ter commented Feb 15, 2018

Thank you for responding, @markerikson. Yes, this makes sense, passing null or undefined as the first argument of connect avoids unnecessarily subscribing for state changes. How about components that do need to subscribe to state changes, is there still an advantage to return state props and dispatch actions separately? Or would it have been just as well to return both kinds of props from a single function? Having access to both state and dispatch, such a function might supply different actions to the wrapped component depending on state.

@markerikson
Copy link
Contributor

markerikson commented Feb 15, 2018

@j18ter : as a simplified explanation, we don't combine them because we're trying to avoid having end users deliberately or accidentally re-recreate functions every time the store state changes. If you absolutely insist on doing so, then you can use the mergeState parameter to connect as a workaround, but we generally discourage its use and that approach overall. Instead, you are encouraged to write separate action creator functions, pass them to connect in the form of an object (the "object shorthand" syntax for mapDispatch), and explicitly call action creators like this.props.someAction(this.props.someValue).

If you need further logic handling, then the "right" place to do it is in a separate thunk function.

@j18ter
Copy link

j18ter commented Feb 15, 2018

@markerikson : Thanks again. I hadn't realized that mapDispatch is only called once, not for every state change as is the case for mapState. Using the object shorthand instead of a passing a mapDispatch function sounds like a good idea too, as it makes this more obvious.

@j18ter
Copy link

j18ter commented Feb 16, 2018

Thanks again for helping me better understand the indended use of the connect wrapper, and apologies for adding comments to a dead issue, but I wanted to return to my original purpose, namely to create a wrapper that connects a pure component to both the Redux state and Meteor data subscriptions. If I were to simply compose connect and withTracker then the function that is passed to withTracker for the purpose of selecting data would not have access to the Redux state, but in some cases it would be nice to subscribe to different data based on state. I've come up with something and wanted to ask for feedback.

Using a slighly different API style I created several wrapping functions for different purposes instead of a single function where one may need to pass undefined for some arguments. Let's start with the case of a component that needs only action dispatchers, but does not depend on Redux state or Meteor server-side data:

export const withActions = actions => wrappedComponent => {
    return connect(undefined, actions)(wrappedComponent);
};

Then there are two wrapping functions for components that depend on either Redux state or Meteor data, but not both:

export const withState = (mapState, actions) => wrappedComponent => {
    return connect(mapState, actions)(wrappedComponent);
};

export const withData = (mapData, actions) => wrappedComponent => {
    const componentToWrap = (actions
                             ? connect(undefined, actions)(wrappedComponent)
                             : wrappedComponent);
    return withTracker(mapData)(componentToWrap);
};

And finally the interesting one, for components that depend on both Redux state and Meteor data, and where the server-side data to be retrieved via Meter's publish/subscribe push API may depend on the state:

export const withDataState = (mapDataState, actions) => wrappedComp => {
    return class ContextWrapper extends React.Component {
        constructor(ownProps, context) {
            super(ownProps, context);
            this.store = ownProps.store || context.store;
            const mapData = props => {
                return mapDataState(this.store.getState(), props);
            };
            this.child = compose(connect(mapDataState, actions),
                                 withTracker(mapData.bind(this)))(wrappedComp);
            this.state = null;
        }
        static contextTypes = {
            store: PropTypes.object.isRequired
        }
        render() {
            return React.createElement(this.child, this.props);
        }
    };
};

This seems to work, but in some cases the mapDataState function is unnecessarily called twice. Perhaps, this could be avoided if withDataState was written in terms of react-redux and meteor-react-data internals instead of the user-level connect and withTracker API, but this may be more than I can manage. Is there anything else that is not optimal here?

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

No branches or pull requests

5 participants