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

Improved mutator API #100

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Improved mutator API #100

wants to merge 15 commits into from

Conversation

smikula
Copy link
Contributor

@smikula smikula commented Jul 6, 2018

This implements most of #97 -- please see that issue for details. (I didn't implement the nullOn or definedOn APIs yet since it's not clear how much value they would add. They can always be added later without breaking the existing API, so I'll hold off.)

@MLoughry
Copy link
Contributor

MLoughry commented Jul 6, 2018

Were there any changes to the API surface as compared to #97 due to our conversations? It would make it easier to follow if you included the updated API documentation as part of this change. #Resolved

Copy link
Contributor

@MLoughry MLoughry left a comment

Choose a reason for hiding this comment

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

I have some real concerns over both the fact that mutator handlers can mutate and/or replace state, rather than simply one or the other, and the performance implications of having every mutator listening for every action.

Additionally, as mentioned in my previous comment, I'd like to see the documentation updates included with this change.

constructor(private initialValue: TState) {}

getInitialValue() {
return this.initialValue;
Copy link
Contributor

@MLoughry MLoughry Jul 6, 2018

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? #Resolved

Copy link
Contributor Author

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)

let handler = this.handlers[actionId];
if (handler) {
let returnValue = handler(currentState, actionMessage);
if (returnValue !== undefined) {
Copy link
Contributor

@MLoughry MLoughry Jul 6, 2018

Choose a reason for hiding this comment

The 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.

  1. It is totally valid to set a value to undefined. However, if you return undefined, then nothing happens (due to seeming black magic)
  2. If you have a handler like so:
    .handles(actionA, (state, action) =>
          state.someProperty = 'A';
      )
    This actually results in replacing the entire state with 'A', rather than merely mutating it. All because of the lack of curly braces. #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. The point about undefined is inarguable and is, I think, a pitfall that is pretty rarely relevant but people will need to be aware of. It's akin to the MobX gotcha around not being able to observe properties that don't exist.
  2. Possible in pure JS, but TypeScript would flag this as an error because your return value would be of the wrong type.

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)


function getMutator<TState>(mutator: TState | LeafMutator<TState> | CombinedMutator<TState>) {
if ((<any>mutator).handleAction) {
return <LeafMutator<TState> | CombinedMutator<TState>>mutator;
Copy link
Contributor

@MLoughry MLoughry Jul 6, 2018

Choose a reason for hiding this comment

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

Would it make sense for LeafMutator and CombinedMutator to implement a common interface? #WontFix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back and forth on this... The problem is that LeafMutator has an extra method that CombinedMutator doesn't, so I still need the package to export that type. I could create an interface for the stuff that's common, but it's just one more type that needs to be exported.


In reply to: 200791352 [](ancestors = 200791352)

return getStore;
}

function getMutator<TState>(mutator: TState | LeafMutator<TState> | CombinedMutator<TState>) {
Copy link
Contributor

@MLoughry MLoughry Jul 6, 2018

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 if {...} else {...} structure that a type guard generally follows. IMHO assigning it to a new mutator variable makes the code clearer.


In reply to: 200791792 [](ancestors = 200791792)


// If necessary, hook the mutator up to the dispatcher
if (mutator) {
subscribeAll(
Copy link
Contributor

@MLoughry MLoughry Jul 6, 2018

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)

try {
getGlobalContext().inMutator = true;
if (target(actionMessage)) {
throw new Error('Mutators cannot return a value and cannot be async.');
Copy link
Contributor

@MLoughry MLoughry Jul 6, 2018

Choose a reason for hiding this comment

The 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 createMutator().handles() handler #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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)

combinedMutator.handleAction({ A: 'a', B: 'b' }, actionMessage, null);

// Assert
expect(spyA.calls.argsFor(0)[0]).toBe('a');
Copy link
Contributor

Choose a reason for hiding this comment

The 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);

@smikula
Copy link
Contributor Author

smikula commented Jul 9, 2018

Just what I mentioned above; I skipped the nullOn and definedOn APIs and suspect we won't really need them. I definitely will get the docs updated, but don't want to do all that writeup until the API is nailed down.


In reply to: 403167816 [](ancestors = 403167816)

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