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

Provide information that action is being replayed #263

Closed
wants to merge 5 commits into from

Conversation

tomkis
Copy link

@tomkis tomkis commented Apr 1, 2016

There are obviously some libraries which are trying to reduce side effects in Reducers (https://github.com/salsita/redux-side-effects, https://github.com/raisemarketplace/redux-loop), however we need to maintain purity of these Reducers - side effect execution is always deferred. This is easy to implement for classic Reducers but we would need to make it work for devtools time travel as well.

There is a workaround and it's basically the way redux-loop is doing that by overriding dispatch drawback of this solution is though that modified dispatch is not used for redux init action and this is for example problem for reproducing Elm architecture in Redux because in Elm architecture there might be side effects in the init function.

That's why we re-wrote redux-side-effects, however this introduces issue with timetravel because we don't have any way to find out when the action is being replayed and when not. This comment describes it quite accurately.

This PR does solve the problem, we need to decide about proper way to implement it though because right now replaying flag is passed as third argument to reducer. The question is though, is that really sane? Can't it break some functionality of other people? Maybe it would be better to provide the information directly in the action itself.

@tomkis
Copy link
Author

tomkis commented Apr 2, 2016

As we had the discussion via twitter, it looks like passing the replaying flag as third argument may break functionality for people, who are using this argument for something else.

redux-devtools instrumentation relies on fact that non-standard actions like thunks for example have already been processed and converted to plain old objects. We can seize the fact and pack the replaying flag directly into the dispatched Action.

Last two commits fixes this.

@@ -70,7 +70,7 @@ function computeNextEntry(reducer, action, state, error, replaying) {
let nextState = state;
let nextError;
try {
nextState = reducer(state, action, replaying);
nextState = reducer(state, { ...action, replaying });
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favour of keeping the original reference untouched, of course it is a performance penalty.

@gaearon
Copy link
Contributor

gaearon commented Apr 2, 2016

Ugh. I kinda feel bad about this. Maybe we should just fix initial dispatch in Redux?
Like we could dispatch lazily the first time getState() is accessed. 😯

@gaearon
Copy link
Contributor

gaearon commented Apr 2, 2016

If you’re up for it, can you look into fixing it in Redux itself? Even if it’s a breaking change. Then we can look at our options again and consider whether it’s worth it.

@gaearon
Copy link
Contributor

gaearon commented Apr 2, 2016

Please see reduxjs/redux#465.
Let’s discuss there instead.

@gaearon gaearon closed this Apr 2, 2016
@tomkis
Copy link
Author

tomkis commented Apr 3, 2016

I thought it was feature not bug that middlewares are not applied for initial dispatch. If that's not the case then I absolutely agree with you!

Thanks, will look into the redux issue now.

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.

2 participants