ReactTraining / react-history

Manage session history with React
266 stars 23 forks source link

Actions are not declarative #25

Closed Rendez closed 6 years ago

Rendez commented 6 years ago

First off, thanks for the great work on the lib.

We have a case where we want to use <Push /> within our components to pushState on tabs change (click to the next active tab). A colleague of mine rightly pointed out that even though we're rendering the push action, we're not really declaring the app state. The reason is that the action component will not change the app state back when it unmounts....

Take this for example:

render() { return this.state.newActiveTab === 'foo' ? <Push path="/foo" /> : null; }.

The correct behavior one would assume, is to "pop" the action when the condition evaluates as false in the above scenario.

We tried to think whether calling history.pop() on cWU would do it, but there can be side effects since other history actions may have been performed by then...

This isn't react-history's fault, rather one related to the stateful history API. Which means we'd have to set <Push path="/" /> both initially on the page (to keep up with URL state), as well as a the default component to render each time we want to go back to the default tab.

We decided action components were a bad idea and opted for creating a HOC instead:

import React, { Component } from 'react';
import { history as historyType } from './PropTypes';
import wrapDisplayName from 'recompose/wrapDisplayName';
import hoistStatics from 'hoist-non-react-statics';
import pick from 'lodash/pick';

export default (WrappedComponent) => {
    class WithHistoryActions extends Component {
        static displayName = wrapDisplayName(
            WrappedComponent,
            'withHistoryActions',
        );

        static contextTypes = {
            history: historyType,
        };

        historyActions = pick(this.context.history, [
            'push',
            'replace',
            'go',
            'goBack',
            'goForward',
            'block',
        ]);

        render() {
            return (
                <WrappedComponent
                    {...this.props}
                    history={this.historyActions}
                />
            );
        }
    }

    return hoistStatics(WithHistoryActions, WrappedComponent);
};

ps: a possible workaround if we want to keep Actions.js is to save the initial location value of createBrowserHistory() in memory and use it to go to that history step when any action component unmounts.

mjackson commented 6 years ago

Yep, I agree that rendering <Push> and <Pop> actions feels weird. This is the main reason that I say "this library is highly experimental" in the README ... because some things like this still feel kinda weird 😅

Thank you for the HOC code. It may help someone else who comes along and has the same question.