History updated with filtered action #106
Comments
|
It looks like connected to #86 and followed. |
|
@xiceph could you provide a small example repo to make it easier to reproduce and fix this issue? something similar to this: https://github.com/peteruithoven/redux-undo-filtering-issue-example would be perfect 😁 |
|
I'll try to separate a basic example. I'll not be easy, it is part of a complex scenario, but maybe just this clear a root of the issue. |
|
I found another way to reproduce the issue, so I easily prepared a small example repo: |
|
hi, is there any solution to this bug? |
|
@AsafShochet unfortunately, not yet... there are a few issues to the current filtering system and no decent solution that fixes all of the problems yet. |
|
If it helps, here's a very small test demonstrating the problem: |
|
Eta on this? My current partial workaround is to keep track of the actions in a stack in the state, and clearHistory() after any filtered action when past.length = 1. |
|
Encountered the same issue too. The filtered action still getting pushed to the past array. |
|
I feel like filters should be labeled as an unfinished/broken feature due to this bug. It breaks the fundamental expected functionality. |
|
I've experimented a bit with version |
|
After going back and forth with different package versions, I am certain that this issue was introduced with version |
|
@joyfulelement I can confirm the exact same problem with beta9. Will wait for fix and use 1.0.0-beta7 until then :) |
|
Without doing a test, I'm pretty sure that 1.0.0-beta7 still has the issue exemplified in https://github.com/xiceph/redux-undo-problem-example but seems to work for my most common use case of having filtered out data-loading actions before non-filtered-out editing actions. |
|
I've fixed this in the following pull request: #139 The basic idea is to save an extra copy of the most recently unfiltered state until a new unfiltered action takes place, or someone clicks undo/redo/jump |
|
I published a new version Let me know if it works now and feel free to reopen if you're still having issues! 😁 |
|
I have the same issue, filtered actions being fired although they are excluded in filter. I'm using beta9-5. |
|
@jide can you check if it works with |
|
beta9-3, beta9-5: Same issue beta7: Works |
|
I'll try to make a reproducible test, I have to find out what's causing this. |
|
I've run the following test on beta9-5 and beta9-3, and it works: import assert from 'assert'
import { describe, it } from 'mocha'
import undoable from 'redux-undo'
const reducer = (state = 0, action) => {
if (action.type === 'INCREMENT') return state + 1
return state
}
const undoableReducer = undoable(reducer, {
filter: () => false
})
describe('check undo stuff', () => {
let result = undoableReducer(undefined, { type: 'INCREMENT' })
it('should ensure state past does not exist', () => {
assert.equal(result.present, 1)
assert.equal(result.past.length, 0)
})
})An example would be very helpful |
|
@jide any word on this? If this isn't an issue anymore, I'll go ahead and close it |
|
hi @davidroeca, I made a failing test, shouldn't both tests should return the same ? import expect from 'expect'
import undoable, { excludeAction, ActionTypes } from 'redux-undo'
function reducer(state = {}, action) {
switch (action.type) {
case 'UNFILTERED_ACTION':
return {
...state,
foo: action.payload
}
case 'FILTERED_ACTION':
return {
...state,
bar: action.payload
}
default:
return state
}
}
describe('check filtered actions', () => {
it('passes', () => {
const undoableReducer = undoable(reducer, {
filter: excludeAction(['FILTERED_ACTION'])
})
let result = undoableReducer(undefined, { type: 'FILTERED_ACTION', payload: 2 })
result = undoableReducer(result, { type: 'UNFILTERED_ACTION', payload: 1 })
result = undoableReducer(result, { type: ActionTypes.UNDO })
result = undoableReducer(result, { type: ActionTypes.UNDO })
expect(result.present).toEqual({ bar: 2 })
expect(result.past.length).toEqual(0)
})
it('fails', () => {
const undoableReducer = undoable(reducer, {
filter: excludeAction(['FILTERED_ACTION'])
})
let result = undoableReducer(undefined, { type: 'UNFILTERED_ACTION', payload: 1 })
result = undoableReducer(result, { type: 'FILTERED_ACTION', payload: 2 })
result = undoableReducer(result, { type: ActionTypes.UNDO })
result = undoableReducer(result, { type: ActionTypes.UNDO })
expect(result.present).toEqual({ bar: 2 })
expect(result.past.length).toEqual(0)
})
}) |
|
edited--I wrote a previous post, but I have a different fix. @jide the fix is to provide an up-front initial state. I'll look into why this is, but it looks like passing undefined and relying on the default arguments of the reducer causes strange behavior. You should be able to undo all the way to the initial state if you provide an empty object {} to the reducer as the starting state: import expect from 'expect'
import undoable, { excludeAction, ActionTypes } from 'redux-undo'
function reducer(state = {}, action) {
switch (action.type) {
case 'UNFILTERED_ACTION':
return {
...state,
foo: action.payload
}
case 'FILTERED_ACTION':
return {
...state,
bar: action.payload
}
default:
return state
}
}
describe('check filtered actions', () => {
it('passes', () => {
const undoableReducer = undoable(reducer, {
filter: excludeAction(['FILTERED_ACTION'])
})
let result = undoableReducer({}, { type: 'FILTERED_ACTION', payload: 2 })
result = undoableReducer(result, { type: 'UNFILTERED_ACTION', payload: 1 })
result = undoableReducer(result, { type: ActionTypes.UNDO })
//result = undoableReducer(result, { type: ActionTypes.UNDO })
expect(result.present).toEqual({ })
expect(result.past.length).toEqual(0)
})
it('fails', () => {
const undoableReducer = undoable(reducer, {
filter: excludeAction(['FILTERED_ACTION'])
})
let result = undoableReducer({}, { type: 'UNFILTERED_ACTION', payload: 1 })
result = undoableReducer(result, { type: 'FILTERED_ACTION', payload: 2 })
result = undoableReducer(result, { type: ActionTypes.UNDO })
result = undoableReducer(result, { type: ActionTypes.UNDO })
expect(result.present).toEqual({ })
expect(result.past.length).toEqual(0)
})
})There are only two things to undo--the change from initial state, and the change caused by the unfiltered action. In fact, you only have to undo once in the first scenario since the state only registers the change caused by the unfiltered action and it's the present state in that case. As for why undefined doesn't work, that might require a deeper look |
|
Found the bug: https://github.com/omnidan/redux-undo/blob/master/src/reducer.js#L172 the other assignments follow the pattern history = config.history = {{initializedHistory}while this one only has history = {{initializedHistory}}This forces the history to get initialized twice |
assign config.history to the initialized history when passed initial state is undefined This fixes inconsistent undo behavior when initial state is undefined, as mentioned in #106
|
@jide @davidroeca I published |
|
Hey ! Hmmm... There's something I do not understand in your updated test: Why would the final state, after the undo(s), be What confuses me even more is that your first test fails, since the final state actually is In my test, I also tried beta9-6 in a project (where Referring to : 🤔 |
|
Yes, that was the goal of my test. The filtering behavior changed from beta-7, so that's probably why you're experiencing an issue where you expect something different. Not sure what you're talking about about the test failing. I pasted that test into the source code of the master branch with the only change being an import switch of 'redux-undo' -> '../src' and every test passed with the addition of my two. Yes, beta 9-6 should provide you the same behavior passing undefined and {} to the undoable reducer because... well... {} is your default argument state in the reducer. The thought behind the filtering behavior is that an unfiltered actions serve as a marker for where you undo back to. So even if a series of actions is filtered (e.g. say you want to filter a 10 actions that fire as a result of one user action), you can undo to the last unfiltered action. For test 1, you should only be able to undo and redo between {foo: 1, bar: 2} and {}, while for test 2, you should only be able to undo and redo between { foo: 1 } and {} since the filtered action's state is lost upon undo. So actually in both tests, you only have to undo once to get to {}. Hope this helps with any confusion--let me know if you have any other questions regarding the new filtering behavior. |
|
Sorry about the failing test, I realized I ran it using beta7... But, to sum things up, using your test :
But actually, the only result that makes sense to me is the first failing test with beta 7. Here is the output if I log after each action : While beta 9-x gives : Here is another test I tried, with an initial state before calling the first action so we can undo it : describe('check filtered actions', () => {
it('passes', () => {
const undoableReducer = undoable(reducer, {
filter: excludeAction(['FILTERED_ACTION'])
})
let result = undoableReducer({})
console.log(result.past, result.present);
result = undoableReducer(result, { type: 'FILTERED_ACTION', payload: 2 })
console.log(result.past, result.present);
result = undoableReducer(result, { type: 'UNFILTERED_ACTION', payload: 1 })
console.log(result.past, result.present);
result = undoableReducer(result, { type: ActionTypes.UNDO })
console.log(result.past, result.present);
result = undoableReducer(result, { type: ActionTypes.UNDO })
console.log(result.past, result.present);
expect(result.present).toEqual({ })
expect(result.past.length).toEqual(0)
})
})Output in beta7 : Output in beta9-x : Maybe there is something I do not understand, but to me, philosophically, triggering 2 consecutive actions that modify the state, and undoing both actions, one filtered and one unfiltered, should result in the state being as if we only triggered the unfiltered action. |
|
Btw, this throws the error I get when using 9-6 in my project : describe('check filtered actions', () => {
it('passes', () => {
const undoableReducer = undoable(reducer, {
filter: excludeAction(['FILTERED_ACTION'])
})
let result = undoableReducer({})
result = undoableReducer({})
result = undoableReducer(result, { type: 'FILTERED_ACTION', payload: 2 })
result = undoableReducer(result, { type: 'UNFILTERED_ACTION', payload: 1 })
expect(result.present).toEqual({ })
expect(result.past.length).toEqual(0)
})
}) |
|
@jide try not to pass subsequent undefined actions to undoable reducer, or you'll experience unexpected behavior. In fact, I wouldn't recommend passing undefined actions to the undoable reducer, period. Always use two arguments. let result = undoableReducer({}) // bad
...
let result = undoableReducer({}, { type: 'UNFILTERED_ACTION', payload: 2 }) // betterWhen fixed, the above test will fail since the present is In response to:
You cannot undo a filtered action. This is by design. When a filtered action occurs, the present state gets updated, but not the past. Hence, if you undo after a filtered action, you'll undo as though your state were that of the latest unfiltered action, and will thus undo to the initial state ({}) in both orderings of filtered -> unfiltered or unfiltered -> filtered. When you undo a filtered action, you can only redo to the latest unfiltered state, so in the filtered -> unfiltered case, you'll be redoing to { bar: 2, foo: 1} and in the unfiltered -> filtered case you'll be redoing only to { foo: 1 }. The following tests should pass: import expect from 'expect'
import undoable, { excludeAction, ActionTypes } from 'redux-undo'
function reducer(state = {}, action) {
switch (action.type) {
case 'UNFILTERED_ACTION':
return {
...state,
foo: action.payload
}
case 'FILTERED_ACTION':
return {
...state,
bar: action.payload
}
default:
return state
}
}
describe('check filtered actions', () => {
it('passes', () => {
const undoableReducer = undoable(reducer, {
filter: excludeAction(['FILTERED_ACTION'])
})
let result = undoableReducer({}, { type: 'FILTERED_ACTION', payload: 2 })
result = undoableReducer(result, { type: 'UNFILTERED_ACTION', payload: 1 })
result = undoableReducer(result, { type: ActionTypes.UNDO })
expect(result.present).toEqual({ })
expect(result.past.length).toEqual(0)
expect(result.future.length).toEqual(1)
result = undoableReducer(result, { type: ActionTypes.REDO })
expect(result.present).toEqual({ bar: 2, foo: 1 })
expect(result.past.length).toEqual(1)
expect(result.future.length).toEqual(0)
})
it('passes', () => {
const undoableReducer = undoable(reducer, {
filter: excludeAction(['FILTERED_ACTION'])
})
let result = undoableReducer({}, { type: 'UNFILTERED_ACTION', payload: 1 })
result = undoableReducer(result, { type: 'FILTERED_ACTION', payload: 2 })
result = undoableReducer(result, { type: ActionTypes.UNDO })
expect(result.present).toEqual({ })
expect(result.past.length).toEqual(0)
expect(result.future.length).toEqual(1)
result = undoableReducer(result, { type: ActionTypes.REDO})
expect(result.present).toEqual({ foo: 1 })
expect(result.past.length).toEqual(1)
expect(result.future.length).toEqual(0)
})
}) |
|
closing for now |
|
@davidroeca : Ok, I get the way it works, I'm surprised to be alone to have my use case. I will create a separate reducer with these actions I guess. Otherwise, I found a fix for the const undoableReducer = undoable(reducer, {
// ...
initTypes: ['@@INIT']
}); |
|
What are you expecting the filter to do for you if it isn't to prevent the past from being updated, and to subsequently prevent you from undoing/redoing a filtered action's state changes? |
|
Undoing when having an unfiltered action and then a filtered action would first undo the unfiltered action and then replay the filtered mutation on the result, keeping its effects. |
const initialState = {}
// {}
let result = undoableReducer(initialState, { type: 'UNFILTERED_ACTION', payload: 1 })
// { unfiltered: 1 }
result = undoableReducer(result, { type: 'FILTERED_ACTION', payload: 'A' })
// { unfiltered: 1, filtered: 'A' }
result = undoableReducer(result, { type: 'UNFILTERED_ACTION', payload: 2 })
// { unfiltered: 2, filtered: 'A' }
result = undoableReducer(result, { type: ActionTypes.UNDO })
// { unfiltered: 1, filtered: 'A' }
result = undoableReducer(result, { type: ActionTypes.UNDO })
// Would do :
// 1: go back to the state before the first UNFILTERED_ACTION
// {}
// 2: Reapply state mutation introduced by FILTERED_ACTION afterwards :
// { filtered: 'A' }So in the end this would be equivalent to not having triggered UNFILTERED_ACTION at all. Which I find logical in the sense of "I have undone UNFILTERED_ACTION, and leave out FILTERED_ACTION from the undo stuff". |
|
So you're expecting that filtered action state changes can never be undone in the context of the reducer. This is essentially the same thing as storing the data in a separate reducer that isn't undoable, so I'd recommend just doing that. |
|
Yes unfortunately it is not always possible to use a separate reducer I think (e.g. when you access this part of the state somewhere in the reducer). In my own case it should be ok though. |
|
At that point, you could also use https://github.com/gaearon/redux-thunk to use logic for the entire state. But I see what you mean, especially if you use another library. This library could benefit from a third categorization of actions, but would also add a decent amount of complexity of this library's code so I don't know if it's worth it to add this feature at this point. Definitely something worth thinking about though. @omnidan thoughts? |
|
Just to chime in, I've hit the same snag as Jide. It won't be too bad to
split a reducer into to, since my non-undoable items aren't referenced much
in the rest of the reducer. But, It would get nice to have Undo allow
definable parts of a reducer that are completely omitted from past/future
arrays, or something to that effect.
…On Tue, Jan 31, 2017 at 9:00 AM, David Roeca ***@***.***> wrote:
At that point, you could also use https://github.com/gaearon/redux-thunk
to use logic for the entire state. But I see what you mean, especially if
you use another library.
This library could benefit from a third categorization of actions, but
would also add a decent amount of complexity of this library's code so I
don't know if it's worth it to add this feature at this point. Definitely
something worth thinking about though. @omnidan
<https://github.com/omnidan> thoughts?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#106 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJF0_wBVo1l7kbwV0AsV6LL9D_Cdwrkks5rX2iXgaJpZM4JYGqM>
.
|
|
You can also compose your reducers at a lower level. Use combineReducers when composing subsections of a reducer that are undoable and not undoable. Just pay attention to the fact that the undoable portion must reference the present. import expect from 'expect'
import { combineReducers } from 'redux'
import undoable, { excludeAction, ActionTypes } from 'redux-undo'
const subReducer1 = (state = 0, action) => {
switch (action.type) {
case 'UNDOABLE_ACTION':
return action.payload
default:
return state
}
}
const subReducer1WithUndo = undoable(subReducer1)
const subReducer2 = (state = 0, action) => {
switch (action.type) {
case 'STATIC_ACTION':
return action.payload
default:
return state
}
}
const reducer = combineReducers({
undoable: subReducer1WithUndo,
static: subReducer2
})
describe('check filtered actions', () => {
it('passes', () => {
let result = reducer(undefined, { type: 'STATIC_ACTION', payload: 2 })
result = reducer(result, { type: 'UNDOABLE_ACTION', payload: 1 })
result = reducer(result, { type: ActionTypes.UNDO })
expect(result.undoable.present).toEqual(0)
expect(result.static).toEqual(2)
result = reducer(result, { type: ActionTypes.REDO })
expect(result.static).toEqual(2)
expect(result.undoable.present).toEqual(1)
})
it('passes', () => {
let result = reducer({}, { type: 'UNDOABLE_ACTION', payload: 1 })
result = reducer(result, { type: 'STATIC_ACTION', payload: 2 })
result = reducer(result, { type: ActionTypes.UNDO })
expect(result.undoable.present).toEqual(0)
expect(result.static).toEqual(2)
result = reducer(result, { type: ActionTypes.REDO})
expect(result.undoable.present).toEqual(1)
expect(result.static).toEqual(2)
})
}) |
|
Likewise, if you need any more specific logic, you can rewrite combineReducers yourself. Check http://redux.js.org/docs/basics/Reducers.html#splitting-reducers and search for the place combineReducers is referenced const reducer = (state, action) => {
// Add additional logic that handles additional complexity.
// Potentially return something different than what you would get from combineReducers
return {
undoable: subReducer1WithUndo(state.undoable, action),
static: subReducer2(state.static, action)
}
} |
I found that filtered action after unfiltered one change history too.
Here is debug output:
action SET_FEATURES is only allowed (not filtered)
As you can see in first SET_HOVERED action changed the history (past array increased to 3 items), however output says: filter prevented action, not storing it.
Just updated to beta9, issue is still there.
The text was updated successfully, but these errors were encountered: