-
Notifications
You must be signed in to change notification settings - Fork 27
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
Improved mutator API #100
base: master
Are you sure you want to change the base?
Improved mutator API #100
Changes from all commits
4510a3e
b2c7e86
aa34cbe
b95c535
0d0af8c
98f065a
6a22e3b
9a303a0
7d04394
a3e4828
e683976
c9de65b
912d38e
70ea4e8
4a255c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import ActionMessage from './interfaces/ActionMessage'; | ||
import MutatorMap from './interfaces/MutatorMap'; | ||
|
||
export default class CombinedMutator<TState extends { [key: string]: any }> { | ||
private mutatorKeys: string[]; | ||
|
||
constructor(private mutatorMap: MutatorMap<TState>) { | ||
this.mutatorKeys = Object.keys(this.mutatorMap); | ||
} | ||
|
||
getInitialValue() { | ||
let initialValue: TState = <TState>{}; | ||
this.mutatorKeys.forEach(key => { | ||
initialValue[key] = this.mutatorMap[key].getInitialValue(); | ||
}); | ||
|
||
return initialValue; | ||
} | ||
|
||
handleAction( | ||
currentState: TState, | ||
actionMessage: ActionMessage, | ||
replaceState: (newState: TState) => void | ||
) { | ||
this.mutatorKeys.forEach(key => { | ||
this.mutatorMap[key].handleAction(currentState[key], actionMessage, newState => { | ||
currentState[key] = newState; | ||
}); | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import ActionCreator from './interfaces/ActionCreator'; | ||
import ActionMessage from './interfaces/ActionMessage'; | ||
import { getPrivateActionId } from './actionCreator'; | ||
|
||
export type MutatorHandler<TState, TAction extends ActionMessage> = ( | ||
state: TState, | ||
action: TAction | ||
) => TState | void; | ||
|
||
// Represents a mutator for a leaf node in the state tree | ||
export default class LeafMutator<TState> { | ||
private handlers: { [actionId: string]: MutatorHandler<TState, ActionMessage> } = {}; | ||
|
||
constructor(private initialValue: TState) {} | ||
|
||
getInitialValue() { | ||
return this.initialValue; | ||
} | ||
|
||
handles<TAction extends ActionMessage>( | ||
actionCreator: ActionCreator<TAction>, | ||
handler: MutatorHandler<TState, TAction> | ||
) { | ||
let actionId = getPrivateActionId(actionCreator); | ||
if (this.handlers[actionId]) { | ||
throw new Error('A mutator may not handle the same action twice.'); | ||
} | ||
|
||
this.handlers[actionId] = handler; | ||
return this; | ||
} | ||
|
||
handleAction( | ||
currentState: TState, | ||
actionMessage: ActionMessage, | ||
replaceState: (newState: TState) => void | ||
) { | ||
let actionId = getPrivateActionId(actionMessage); | ||
let handler = this.handlers[actionId]; | ||
if (handler) { | ||
let returnValue = handler(currentState, actionMessage); | ||
if (returnValue !== undefined) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is part of the reason I don't like the ability to return a replacement value, especially in addition to mutating the value.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My personal feeling is that these downsides aren't strong enough to clutter up the API with two different methods, but that would be the alternative. I'd like to gather a few more opinions and then settle on which way to do it. In reply to: 200790946 [](ancestors = 200790946) |
||
replaceState(<TState>returnValue); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import MutatorMap from './interfaces/MutatorMap'; | ||
import CombinedMutator from './CombinedMutator'; | ||
|
||
export default function combineMutators<TState>(mutatorMap: MutatorMap<TState>) { | ||
return new CombinedMutator(mutatorMap); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import LeafMutator from './LeafMutator'; | ||
|
||
export default function createMutator<TState>(initialValue: TState) { | ||
return new LeafMutator(initialValue); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,9 @@ | ||
import { action } from 'mobx'; | ||
import getRootStore from './getRootStore'; | ||
import CombinedMutator from './CombinedMutator'; | ||
import LeafMutator from './LeafMutator'; | ||
import { subscribeAll } from './dispatcher'; | ||
import wrapMutator from './wrapMutator'; | ||
|
||
let createStoreAction = action('createStore', function createStoreAction( | ||
key: string, | ||
|
@@ -8,7 +12,36 @@ let createStoreAction = action('createStore', function createStoreAction( | |
getRootStore().set(key, initialState); | ||
}); | ||
|
||
export default function createStore<T>(key: string, initialState: T): () => T { | ||
export default function createStore<TState>( | ||
key: string, | ||
arg2: TState | LeafMutator<TState> | CombinedMutator<TState> | ||
): () => TState { | ||
// Get the initial state (from the mutator, if necessary) | ||
let mutator = getMutator(arg2); | ||
let initialState = mutator ? mutator.getInitialValue() : arg2; | ||
|
||
// Create the store under the root store | ||
createStoreAction(key, initialState); | ||
return () => <T>getRootStore().get(key); | ||
let getStore = () => <TState>getRootStore().get(key); | ||
|
||
// If necessary, hook the mutator up to the dispatcher | ||
if (mutator) { | ||
subscribeAll( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems really non-performant, if every mutator created in this new API is listening for every action. #Resolved There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this is all just pure JS and that for mutators that don't care about the action it's basically a NOP, I'm not too worried. If it does turn out to be a problem I could imagine a way to have the mutators expose what actions they care about so we could subscribe to them individually...but I'm not going to add that complexity unless we know it's a problem. In reply to: 200791963 [](ancestors = 200791963) |
||
wrapMutator(actionMessage => { | ||
mutator.handleAction(getStore(), actionMessage, newState => { | ||
getRootStore().set(key, newState); | ||
}); | ||
}) | ||
); | ||
} | ||
|
||
return getStore; | ||
} | ||
|
||
function getMutator<TState>(mutator: TState | LeafMutator<TState> | CombinedMutator<TState>) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make more sense to implement a type guard method instead? function getMutator<TState>(mutator: TState | LeafMutator<TState> | CombinedMutator<TState>): mutator is LeafMutator<TState> | CombinedMutator<TState> {
let typedMutator = <LeafMutator<TState> | CombinedMutator<TState>>mutator;
return !!(typedMutator.handleAction && typedMutator.getInitialValue);
} #WontFix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could, but the code here doesn't lend itself so well to the In reply to: 200791792 [](ancestors = 200791792) |
||
if ((<any>mutator).handleAction) { | ||
return <LeafMutator<TState> | CombinedMutator<TState>>mutator; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make sense for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I went back and forth on this... The problem is that In reply to: 200791352 [](ancestors = 200791352) |
||
} | ||
|
||
return null; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import LeafMutator from '../LeafMutator'; | ||
import CombinedMutator from '../CombinedMutator'; | ||
|
||
type MutatorMap<TState extends { [key: string]: any }> = { | ||
[K in keyof TState]: LeafMutator<TState[K]> | CombinedMutator<TState[K]> | ||
}; | ||
|
||
export default MutatorMap; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
import { action } from 'mobx'; | ||
import ActionMessage from './interfaces/ActionMessage'; | ||
import MutatorFunction from './interfaces/MutatorFunction'; | ||
import { getGlobalContext } from './globalContext'; | ||
|
||
// Wraps the target function for use as a mutator | ||
export default function wrapMutator<T extends ActionMessage>( | ||
target: MutatorFunction<T> | ||
): MutatorFunction<T> { | ||
// Wrap the target in a MobX action so it can modify the store | ||
return action((actionMessage: T) => { | ||
try { | ||
getGlobalContext().inMutator = true; | ||
if (target(actionMessage)) { | ||
throw new Error('Mutators cannot return a value and cannot be async.'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to contradict the fact that you can return a value in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I can see how this is confusing. Effectively this will only ever apply to the existing mutators (which are functions), not the new-style mutators (which are objects that can have handler functions registered onto them). Fortunately there should be no confusion to the consumer -- if they're using the new style mutators they'll never see this, and if they do see it with an old-style mutator the callstack will make it obvious where it's coming from. In reply to: 200792183 [](ancestors = 200792183) |
||
} | ||
} finally { | ||
getGlobalContext().inMutator = false; | ||
} | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,59 @@ | ||
import { actionCreator } from '../src/index'; | ||
import CombinedMutator from '../src/CombinedMutator'; | ||
import createMutator from '../src/createMutator'; | ||
|
||
describe('CombinedMutator', () => { | ||
const mutatorA = createMutator('a'); | ||
const mutatorB = createMutator('b'); | ||
|
||
it('combines the initial values of its child mutators', () => { | ||
// Act | ||
let combinedMutator = new CombinedMutator({ | ||
A: mutatorA, | ||
B: mutatorB, | ||
}); | ||
|
||
// Assert | ||
expect(combinedMutator.getInitialValue()).toEqual({ A: 'a', B: 'b' }); | ||
}); | ||
|
||
it('dispatches actions to each child mutator', () => { | ||
// Arrange | ||
const actionMessage = {}; | ||
const spyA = spyOn(mutatorA, 'handleAction'); | ||
const spyB = spyOn(mutatorB, 'handleAction'); | ||
|
||
let combinedMutator = new CombinedMutator({ | ||
A: mutatorA, | ||
B: mutatorB, | ||
}); | ||
|
||
// Act | ||
combinedMutator.handleAction({ A: 'a', B: 'b' }, actionMessage, null); | ||
|
||
// Assert | ||
expect(spyA.calls.argsFor(0)[0]).toBe('a'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: You can write these asserts as expect(spyA).toHaveBeenCalledWith('a', actionMessage);
expect(spyB).toHaveBeenCalledWith('b', actionMessage); |
||
expect(spyA.calls.argsFor(0)[1]).toBe(actionMessage); | ||
expect(spyB.calls.argsFor(0)[0]).toBe('b'); | ||
expect(spyB.calls.argsFor(0)[1]).toBe(actionMessage); | ||
}); | ||
|
||
it('replaces a child state when the replaceState callback gets called', () => { | ||
// Arrange | ||
spyOn( | ||
mutatorA, | ||
'handleAction' | ||
).and.callFake((state: any, actionMessage: any, replaceState: Function) => { | ||
replaceState('x'); | ||
}); | ||
|
||
let state = { A: 'a' }; | ||
let combinedMutator = new CombinedMutator({ A: mutatorA }); | ||
|
||
// Act | ||
combinedMutator.handleAction(state, {}, null); | ||
|
||
// Assert | ||
expect(state.A).toBe('x'); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a concern that if a mutable value is passed, the
initialValue
could change over time? #ResolvedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point. No, I wasn't worried about this because MobX creates a clone of the value rather than allowing the original value to be modified. However, this is only true of more recent versions of MobX. So this could be busted in the v3 branch of satchel because it allows use of MobX 2. That's a bummer, because I had intended to cherry-pick this back into v3. I'm not sure if this is a big enough gotcha to avoid doing that or not.
In reply to: 200790349 [](ancestors = 200790349)