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

Have dispatch return the action it's provided after the current loop concludes #205

Merged
merged 1 commit into from
Jan 16, 2019

Conversation

erik-singleton
Copy link
Contributor

The vanilla implementation of redux passes the provided action back as a convenience. The doc comment does mention that this isn't guaranteed behavior across middleware/store enhancers, but during our transition to redux-loop from redux-thunk, this characteristic of dispatch is important because it was pretty heavily relied on in the past.

Just looking at the code, it's unclear why we do not return the provided action. Especially if there are multiple actions in the cmdRun queue, we seem to discard the result of the Promises.

Applying this change has not broken our limited usage of redux-loop, and I'm curious if it may break in other places.

Thanks!

@bdwain
Copy link
Member

bdwain commented Dec 19, 2018

Hi @sasucker. Thanks for the PR.

Can you explain a little more what you need the return value for? Especially because vanilla redux returns the value synchronously and this would return asynchronously, it seems like you'd have to change all of your code anyway.

@erik-singleton
Copy link
Contributor Author

erik-singleton commented Dec 19, 2018

Hi @bdwain, thanks for getting back so quickly!

As mentioned, we previously (and currently) use redux-thunk for our async behavior. For better or for worse, some of our older components rely on the underlying Promise that is returned from an async thunk.

To illustrate:

function asyncAction() {
    return (dispatch) => {
        dispatch(fetching());
        return request().then(({data}) => {
            dispatch(complete(data));
            return data;
        });
    };
}

// although in actuality we'd use `bindActionCreators`, which also returns the `dispatch`
class SomeComponent {
    componentDidMount() {
        this.setState({loading: true});

        this.props.store.dispatch(asyncAction()).then((datas) => {
            this.setState({datas, loading: false});
        });
    }

    render() {
        return this.state.loading
            ? <h1>{this.state.datas}</h1>
            : <LoadingIndicator />;
    }
};

We've largely moved away from this pattern, but in the interest of transitioning it would definitely help us if redux-loop's dispatch also resolved with the provided action.

@bdwain
Copy link
Member

bdwain commented Jan 16, 2019

Got it. Thanks for explaining. I don't see this being a problem since we currently always return undefined.

Sorry I couldn't get back sooner. I was pretty busy over the holidays.

@bdwain bdwain merged commit 77db34d into redux-loop:master Jan 16, 2019
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 this pull request may close these issues.

None yet

3 participants