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.

Sign up for GitHub

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

Jump to bottom

Distinct state fixes #59

Closed
wants to merge 2 commits into from
Closed

Distinct state fixes #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61e9c8a
Don't always filter distinct states
teameh Mar 11, 2016
7032876
Fixed tests for distinct state
teameh Mar 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,9 +207,11 @@ export default function undoable (reducer, rawConfig = {}) {
return (state, action = {}) => {
debugStart(action, state)

let justCreatedHistory = false
let history
if (!config.history) {
debug('create history on init')
justCreatedHistory = true
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Set flag for later


if (state === undefined) {
config.history = createHistory(reducer(state, {}))
Expand Down Expand Up @@ -289,11 +291,12 @@ export default function undoable (reducer, rawConfig = {}) {
}
}

if (history.present !== res) {
// Don't add entry to history if it was just created
if ((justCreatedHistory && history.present === res) || action.type === '@@redux-undo/INIT') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

gaearon has stated in reduxjs/redux#186 that one day they may change the name of @@redux/INIT to a randomized action name, so if we check it in any way it could be broken by an update in Redux.

If we use a flag that is set to true after the first run of the reducer, then a hot-reload which calls @@redux/INIT will have an incorrect flag and will call an extra insert.

I'm still struggling with this one 👻

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(Think you mean @@redux/INITin your first paragraph righ?)

Crap.. good point.

If we use a flag that is set to true after the first run of the reducer, then a hot-reload which calls @@redux/INIT will have an incorrect flag and will call an extra insert.

The flag is in the scope of the function, so that won't cause an extra insert right?

And HMR will always replace the whole reducers and reapply our decorator around the recuder right?

module.hot.accept('../reducers', () => {
  const nextRootReducer = require('../reducers').default
  store.replaceReducer(nextRootReducer)
})

But if we want to reset to the history upon @@redux/INIT to work with HMR then we really gonna have to have that action hardcoded right? I think it would be fair to just add both @@redux/INIT and @@redux-undo/INIT in our initTypes config and tell the users that f they remove those types:

  • HMR will be broken
  • Additional states will be added to the history if those actions are called
  • Users could prevent that last side effect by adding a custom filter that filters those actions :)

Like this: c5bc86b

And on HMR, why don't we create some functional tests with HMR and webpack, haha that would be a nightmare. No, just kidding. Gaearon just wrote a nice longread on the current state of HMR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pff, I've spend some more hours on this but cannot get it right..

I think we do have some big problems with HMR.
I've added some test that show that replacing reducers is not working how users might expect it.

See the distinct_state_fixes_more_fixes branch: 29a3e7e

Maybe you could check the comments out and let me know what you think. I've added some comments in the code...

Copy link
Owner

Choose a reason for hiding this comment

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

Just a note, we are using @@redux-undo/INIT here, not @@redux/INIT. We can deal with our own init action in any way we please, just don't touch @@redux/INIT 😉

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Well that is the question.. you think you could make HMR with both initial state from stores and reducers work without touching @@redux/INIT? I'm not sure if I know how.

debug('not inserted, history was just created or action === redux-undo/INIT')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So yeah.. I reintroduced the INIT action here. Cause we never want to insert when that is fired right?
And we also don't want to insert when we just created the history (Redux init)

All other actions will be recorded again, so also the actions resulting in the same state as before. As long as the distinctState filter is not active. I think that is the preferred behavior.

} else {
history = insert(history, res, config.limit)
debug('inserted new state into history')
} else {
debug('not inserted, history is unchanged')
}

debug('history: ', history, ' free: ', config.limit - length(history))
Expand Down
21 changes: 17 additions & 4 deletions test/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const { expect } = require('chai')
const { default: undoable, ActionCreators, excludeAction, isHistory } = require('../src/index')
const { default: undoable, ActionCreators, excludeAction, distinctState, isHistory } = require('../src/index')
const Redux = require('redux')

const excludedActionsOne = ['DECREMENT']
Expand Down Expand Up @@ -27,6 +27,8 @@ const initialStateTwo = {

const testConfigThree = {
limit: -1,
filter: distinctState(),
FOR_TEST_ONLY_distinctState: true,
initTypes: []
}
const initialStateThree = {
Expand All @@ -50,7 +52,6 @@ function runTestWithConfig (testConfig, initialStoreState, label) {
let mockUndoableReducer
let mockInitialState
let incrementedState
let doubleIncrementedState
let countReducer
let store

Expand All @@ -62,6 +63,8 @@ function runTestWithConfig (testConfig, initialStoreState, label) {
return state + 1
case 'DECREMENT':
return state - 1
case 'DUMMY':
return state
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added action without state changes, oh maybe that was not necessary... Well it nice and verbose now.. right :D ?

default:
return state
}
Expand All @@ -73,13 +76,11 @@ function runTestWithConfig (testConfig, initialStoreState, label) {

mockInitialState = mockUndoableReducer(undefined, {})
incrementedState = mockUndoableReducer(mockInitialState, { type: 'INCREMENT' })
doubleIncrementedState = mockUndoableReducer(incrementedState, { type: 'INCREMENT' })
console.info(' Beginning Test! Good luck!')
console.info(' initialStoreState: ', initialStoreState)
console.info(' store.getState(): ', store.getState())
console.info(' mockInitialState: ', mockInitialState)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved those down, only needed once

console.info(' incrementedState: ', incrementedState)
console.info(' doubleIncrementedState:', doubleIncrementedState)
console.info('')

expect(store.getState()).to.deep.equal(mockInitialState, 'mockInitialState should be the same as our store\'s state')
Expand Down Expand Up @@ -118,6 +119,17 @@ function runTestWithConfig (testConfig, initialStoreState, label) {
}
})

it('should record non state changing actions when filter is not set', () => {
let dummyState = mockUndoableReducer(incrementedState, { type: 'DUMMY' })
if (testConfig.FOR_TEST_ONLY_distinctState) {
expect(dummyState).to.deep.equal(incrementedState)
} else {
expect(dummyState.present).to.deep.equal(incrementedState.present)
expect(dummyState.past).to.deep.equal([...incrementedState.past, incrementedState.present])
expect(dummyState.future).to.deep.equal(incrementedState.future)
}
})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And a test for distinct state filter, will increase coverage to 100% when the other is merged :)

it('should reset upon init actions', () => {
let reInitializedState
if (testConfig.initTypes) {
Expand Down Expand Up @@ -302,6 +314,7 @@ function runTestWithConfig (testConfig, initialStoreState, label) {
let doubleUndoState
let doubleRedoState
before('perform a jump action', () => {
let doubleIncrementedState = mockUndoableReducer(incrementedState, { type: 'INCREMENT' })
jumpToPastState = mockUndoableReducer(doubleIncrementedState, ActionCreators.jump(jumpStepsToPast))
jumpToFutureState = mockUndoableReducer(mockInitialState, ActionCreators.jump(jumpStepsToFuture))
doubleUndoState = mockUndoableReducer(doubleIncrementedState, ActionCreators.undo())
Expand Down