Skip to content

Commit 323078e

Browse files
leoasisleoasis
committed
Avoid using lodash. Check for Immutable types
1 parent 57bcae6 commit 323078e

File tree

3 files changed

+162
-67
lines changed

3 files changed

+162
-67
lines changed

‎package.json

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,16 +51,19 @@
5151
"react-hot-loader": "^1.2.7",
5252
"rimraf": "^2.3.4",
5353
"webpack": "^1.9.6",
54-
"webpack-dev-server": "^1.8.2"
54+
"webpack-dev-server": "^1.8.2",
55+
"immutable": "~3.7.4"
5556
},
5657
"dependencies": {
5758
"invariant": "^2.0.0"
5859
},
5960
"npmName": "redux",
60-
"npmFileMap": [{
61-
"basePath": "/dist/",
62-
"files": [
63-
"*.js"
64-
]
65-
}]
61+
"npmFileMap": [
62+
{
63+
"basePath": "/dist/",
64+
"files": [
65+
"*.js"
66+
]
67+
}
68+
]
6669
}

‎src/middleware/warnMutations.js

Lines changed: 57 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,65 @@
11
import invariant from 'invariant';
2+
import isPlainObject from '../utils/isPlainObject';
23

3-
import isEqual from 'lodash/lang/isEqual';
4-
import any from 'lodash/collection/any';
5-
import cloneDeep from 'lodash/lang/cloneDeep';
4+
function any(collection, predicate) {
5+
for (let key in collection) {
6+
if (collection.hasOwnProperty(key)) {
7+
if (predicate(collection[key], key)) {
8+
return true;
9+
}
10+
}
11+
}
12+
return false;
13+
}
614

7-
function copyState(state) {
8-
return cloneDeep(state);
15+
// Based on https://github.com/facebook/immutable-js/issues/421#issuecomment-87089399
16+
function isImmutableDefault(value) {
17+
return typeof value !== 'object' ||
18+
(typeof value.equals === 'function' && typeof value.hashCode === 'function');
919
}
1020

11-
function wasMutated(prevStateRef, prevState, stateRef, state) {
12-
if (prevStateRef === stateRef && !isEqual(prevState, state)) {
13-
return true;
21+
function copyState(state, isImmutable) {
22+
if (!state) { return state; }
23+
24+
if (isImmutable(state)) {
25+
return state;
26+
}
27+
28+
if (!Array.isArray(state) && !isPlainObject(state)) {
29+
return state;
30+
}
31+
32+
const keysAndValues = [];
33+
34+
for (let key in state) {
35+
if (state.hasOwnProperty(key)) {
36+
keysAndValues.push([key, copyState(state[key], isImmutable)]);
37+
}
1438
}
1539

16-
if (typeof prevStateRef !== 'object') {
40+
const initialObj = Array.isArray(state) ? [] : {};
41+
42+
return keysAndValues.reduce((obj, [key, value]) => {
43+
obj[key] = value;
44+
return obj;
45+
}, initialObj);
46+
}
47+
48+
function wasMutated(prevStateRef, prevState, stateRef, state, isImmutable, sameParentRef = true) {
49+
if (isImmutable(prevState)) {
50+
if (sameParentRef) {
51+
return prevState !== state;
52+
}
53+
1754
return false;
1855
}
1956

2057
return any(prevStateRef, (val, key) =>
21-
wasMutated(val, prevState[key], stateRef[key], state[key]));
58+
wasMutated(
59+
val, prevState[key], stateRef[key], state[key],
60+
isImmutable, prevStateRef === stateRef
61+
)
62+
);
2263
}
2364

2465
const BETWEEN_DISPATCHES_MESSAGE = [
@@ -33,26 +74,26 @@ const INSIDE_DISPATCH_MESSAGE = [
3374
'(https://github.com/gaearon/redux#my-views-arent-updating)'
3475
].join('');
3576

36-
export default function warnMutationsMiddleware(getState) {
77+
export default function warnMutationsMiddleware(getState, isImmutable = isImmutableDefault) {
3778
let lastStateRef = getState();
38-
let lastState = copyState(lastStateRef);
79+
let lastState = copyState(lastStateRef, isImmutable);
3980

4081
return (next) => (action) => {
4182
const stateRef = getState();
42-
const state = copyState(stateRef);
83+
const state = copyState(stateRef, isImmutable);
4384

4485
invariant(
45-
!wasMutated(lastStateRef, lastState, stateRef, state),
86+
!wasMutated(lastStateRef, lastState, stateRef, state, isImmutable),
4687
BETWEEN_DISPATCHES_MESSAGE
4788
);
4889

4990
const dispatchedAction = next(action);
5091

5192
lastStateRef = getState();
52-
lastState = copyState(lastStateRef);
93+
lastState = copyState(lastStateRef, isImmutable);
5394

5495
invariant(
55-
!wasMutated(stateRef, state, lastStateRef, lastState),
96+
!wasMutated(stateRef, state, lastStateRef, lastState, isImmutable),
5697
INSIDE_DISPATCH_MESSAGE,
5798
action.type
5899
);

‎test/middleware/warnMutations.spec.js

Lines changed: 95 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,73 +1,124 @@
11
import expect from 'expect';
2+
import Immutable from 'immutable';
23
import warnMutationsMiddleware from '../../src/middleware/warnMutations';
34

45
describe('middleware', () => {
56
describe('warnMutationsMiddleware', () => {
67
let state;
78
const getState = () => state;
89

9-
it('should send the action through the middleware chain', () => {
10-
state = {foo: {bar: [2, 3, 4], baz: 'baz'}};
11-
const next = action => action;
12-
const dispatch = warnMutationsMiddleware(getState)(next);
10+
function middleware(next) {
11+
return warnMutationsMiddleware(getState)(next);
12+
}
1313

14-
expect(dispatch({type: 'SOME_ACTION'})).toEqual({type: 'SOME_ACTION'});
15-
});
14+
function testCasesForMutation(mutation) {
15+
it('should throw if happening inside the dispatch', () => {
16+
const next = action => {
17+
state = mutation(state);
18+
return action;
19+
};
1620

17-
it('should throw if there is a state mutation inside the dispatch', () => {
18-
state = {foo: {bar: [2, 3, 4], baz: 'baz'}};
21+
const dispatch = middleware(next);
1922

20-
const next = action => {
21-
state.foo.baz = 'changed!';
22-
return action;
23-
};
23+
expect(() => {
24+
dispatch({type: 'SOME_ACTION'});
25+
}).toThrow();
26+
});
2427

25-
const dispatch = warnMutationsMiddleware(getState)(next);
28+
it('should throw if happening between dispatches', () => {
29+
const next = action => action;
2630

27-
expect(() => {
28-
dispatch({type: 'SOME_ACTION'});
29-
}).toThrow();
30-
});
31+
const dispatch = middleware(next);
3132

32-
it('should not throw if dispatch returns a new state object', () => {
33-
state = {foo: {bar: [2, 3, 4], baz: 'baz'}};
33+
dispatch({type: 'SOME_ACTION'});
34+
state = mutation(state);
35+
expect(() => {
36+
dispatch({type: 'SOME_OTHER_ACTION'});
37+
}).toThrow();
38+
});
39+
}
3440

35-
const next = action => {
36-
state = {...state, foo: {...state.foo, baz: 'changed!'}};
37-
return action;
38-
};
41+
function testCasesForNonMutation(nonMutation) {
3942

40-
const dispatch = warnMutationsMiddleware(getState)(next);
43+
it('should not throw if happening inside the dispatch', () => {
44+
const next = action => {
45+
state = nonMutation(state);
46+
return action;
47+
};
4148

42-
expect(() => {
43-
dispatch({type: 'SOME_ACTION'});
44-
}).toNotThrow();
45-
});
49+
const dispatch = middleware(next);
4650

47-
it('should throw if a state mutation happened between dispatches', () => {
48-
state = {foo: {bar: [2, 3, 4], baz: 'baz'}};
49-
const next = action => action;
51+
expect(() => {
52+
dispatch({type: 'SOME_ACTION'});
53+
}).toNotThrow();
54+
});
5055

51-
const dispatch = warnMutationsMiddleware(getState)(next);
56+
it('should not throw if happening between dispatches', () => {
57+
const next = action => action;
5258

53-
dispatch({type: 'SOME_ACTION'});
54-
state.foo.baz = 'changed!';
59+
const dispatch = middleware(next);
5560

56-
expect(() => {
57-
dispatch({type: 'SOME_OTHER_ACTION'});
58-
}).toThrow();
61+
dispatch({type: 'SOME_ACTION'});
62+
state = nonMutation(state);
63+
expect(() => {
64+
dispatch({type: 'SOME_OTHER_ACTION'});
65+
}).toNotThrow();
66+
});
67+
}
68+
69+
beforeEach(() => {
70+
state = {foo: {bar: [2, 3, 4], baz: 'baz', qux: Immutable.fromJS({a: 1, b: 2})}};
5971
});
6072

61-
it('should not throw if there weren\'t any state mutations between dispatches', () => {
62-
state = {foo: {bar: [2, 3, 4], baz: 'baz'}};
73+
it('should send the action through the middleware chain', () => {
6374
const next = action => action;
75+
const dispatch = middleware(next);
6476

65-
const dispatch = warnMutationsMiddleware(getState)(next);
77+
expect(dispatch({type: 'SOME_ACTION'})).toEqual({type: 'SOME_ACTION'});
78+
});
79+
80+
const mutations = {
81+
'mutating nested array': (s) => {
82+
s.foo.bar.push(5);
83+
return s;
84+
},
85+
'mutating nested array and setting new root object': (s) => {
86+
s.foo.bar.push(5);
87+
return {...s};
88+
},
89+
'changing nested string': (s) => {
90+
s.foo.baz = 'changed!';
91+
return s;
92+
},
93+
'setting a nested immutable object': (s) => {
94+
s.foo.qux = s.foo.qux.set('a', 3);
95+
return s;
96+
}
97+
};
98+
99+
Object.keys(mutations).forEach((mutationDesc) => {
100+
describe(`mutating state by ${mutationDesc}`, () => {
101+
testCasesForMutation(mutations[mutationDesc]);
102+
});
103+
});
66104

67-
dispatch({type: 'SOME_ACTION'});
68-
expect(() => {
69-
dispatch({type: 'SOME_OTHER_ACTION'});
70-
}).toNotThrow();
105+
const nonMutations = {
106+
'returning same state': (s) => s,
107+
'returning a new state object with nested new string': (s) => {
108+
return {...s, foo: {...s.foo, baz: 'changed!'}};
109+
},
110+
'returning a new state object with nested new array': (s) => {
111+
return {...s, foo: {...s.foo, bar: [...s.foo.bar, 5]}};
112+
},
113+
'returning a new state object with nested new immutable state': (s) => {
114+
return {...s, foo: {...s.foo, qux: s.foo.qux.set('a', 3)}};
115+
}
116+
};
117+
118+
Object.keys(nonMutations).forEach((nonMutationDesc) => {
119+
describe(`not mutating state by ${nonMutationDesc}`, () => {
120+
testCasesForNonMutation(nonMutations[nonMutationDesc]);
121+
});
71122
});
72123
});
73124
});

0 commit comments

Comments
 (0)