Adding a way for @dispatch methods to return a value that won't trigger a dispatch

[rart]

Adding a way for methods that use the @dispatch decorator to return a value that won't trigger a dispatch so they can conditionally dispatch based on any logic that suits the need.

Currently returning anything but an action would cause middleware like redux-observable to throw or returning a mock action like { type: 'noop' } would still activate the root reducer that, even though technically nothing will happen, just seems nicer that you can avoid all together the store.dispatch from within the same @dispatch-decorated method.

By having a specific value to allow this behaviour — rather than just using null or undefined — prevents accidental development errors of developers just forgetting to return.

[e-schultz]

Hi,

Thanks for the PR - I will give this some thought, although situations like this is making me question "was adding @dispatch a great idea in the first place?" - as I've run into similar conundrums in my own projects and/or with people calling methods that self-dispatch, but say within a redux-observable Epic (epic dispatches the method - but so does the @dispatch decorator)

Feels like if something is decorated with @dispatch - it should always dispatch, and be upto the callers to decide if they want to dispatch it or not.

eg:

export class Component {
  handleSubmit(form: any) {
    if (formValid) {
      this.doSubmit(form)
    }
  }
  @dispatch()
  doSubmit(form: any) {
    return { type: 'FORM_SUBMITTED', payload: form };
  }
}


// instead of

export class Component {
 
  @dispatch()
  doSubmit(form: any) {
    if(formValid) {
      return { type: 'FORM_SUBMITTED', payload: form };
    }
    else {
      return { type: NgRedux.DISPATCH_NOOP }
    }
  }
}

@danielfigueiredo @smithad15 @SethDavenport thoughts?

[rart]

Totally understand where you're coming from. I've asked myself the same question. Your example is great because it demonstrates the kind of scenario where I think this should be used: very simple quick checks.

I found my self doing several similar little checks like those where I feel an additional function to handle is unnecessary and... boilerplatey? ..just more code that it needs be. One could even do it in the template but I think is nice and cleaner to do it this way e.g.

@dispatch()
selectTab(tab) {
  return (this.active.id !== tab.id) 
    ? { type: .... } 
    : { type: 'noop' } // or NgRedux.DISPATCH_NOOP eventually
}

Ultimately, this gives the flexibility for people to use it however they choose. If they have strong philosophical opinions they could use it for super simple checks like the example or avoid it all together; if they choose to use it differently they have the option too. Unless you specifically want to angular-redux to be opinionated for this matter, the change is minimal and provides a nice convenience.

Other tiny detail I was wondering was what the name of the constant should be. I ended up submitting DISPATCH_NOOP but NO_DISPATCH or others where high up in my list hehe

Thanks for considering :)

[danielfigueiredo]

Trying to find a middle ground here, I personally don't feel like having an action NOOP that NgRedux checks for in order to figure out whether to dispatch
Then I thought, perhaps a clear way to support what you're proposing without adding any actions is:

if (store && result) {
  store.dispatch(result);
}

But looking at reactjs/redux it's very clear that we can't fire null actions or anything alike:

  function dispatch(action) {
    if (!isPlainObject(action)) {
      throw new Error(
        'Actions must be plain objects. ' +
          'Use custom middleware for async actions.'
      )
    }

    if (typeof action.type === 'undefined') {
      throw new Error(
        'Actions may not have an undefined "type" property. ' +
          'Have you misspelled a constant?'
      )
    }
...
}

Thus, doing this make me feel like I'm going against redux. They intentionally fail any attempt to fire an invalid action (not a POJO, missing type prop, null | undefined, etc), changing this behaviour seems like we are not being canonical to redux. I think that restructuring the code might be a better way out of the problem instead of moving to ngRedux, if the handle introduces boilerplate perhaps you could just dispatch manually

selectTab(tab) {
  if (this.active.id !== tab.id) { 
    this.ngRedux.dispatch(action);
  }
}

^ this feels cleaner to me than dispatching noop in an else, but that is just my personal opinion

[rart]

I thought of returning false, undefined or null but I didn't like either of those, especially undefined or null introduces the possibility that the programmer just forgets to return and it sort of "fails" silently. An explicit value for doing this enforces you to either return an action or a known value for NgRedux to know you want to avoid dispatching all together.

I don't feel this goes against redux. The whole point is for this to avoid a non-action or dummy one getting to redux (and middleware) in the first place. Exactly how do you think this goes against it; which redux principle does this go against?

The idea is to add an additional convenience into the library that gives safe concise code through @dispatch. Dispatching manually:

1. forces you to include the store to your constructor (again, boilerplate-ey)
2. introduces inconsistent dispatching mechanisms within the same component
3. makes you lose the convenient visibility of knowing that a method dispatches when you're just scanning through the code rapidly or specially when you are a heavy user of code folding like myself.

I was also considering the possibility of @dispatch() receiving some sort of argument @dispatch(() => { ... }) where logic could decide whether to dispatch or so. Thought that was an interesting option too.

[rart]

You guys seem to agree that this shouldn't go into NgRedux so I've started thinking on what am I going to do to achieve this and thought I may just create my own @dispatch and import it instead of NgRedux's or a brand new one like @mayDispatch, @dispatcher — or something like that... Thought I'd mention the option in case that resonates more: create a different one that more semantically states it conditionally dispatches. I still like the original option, though haha.

[SethDavenport]

Hey, late to the party, sorry. I've had very limited OSS time since I changed jobs :(

That said my original intention with store was to have two 'interfaces':

the "regular one" where you inject store and call select, dispatch directly in your classes,

the "decalarative one" (aka the Decorator interface) for people like my who are obsessed with declarative components. This is largely what the decorators are for.

The idea is that if you're doing simple stuff, the decorator interface is helpful, but if you're doing complicated stuff you always have the power of injecting store and using it more imperatively. Or going with epics. Or whatever.

I personally am fairly opinionated but I don't necessarily want to bake that into the API because who am I to tell people how to code? :). I don't want to limit good ideas that I haven't thought about.

For your use case I'd be cool with allowing the decorated function to return null or undefined: probably can just do a !!result check here: https://github.com/angular-redux/store/blob/master/src/decorators/dispatch.ts#L21

That way we just need to update the API doc to spell out that @dispatch() does nothing to a falsy value and you can use it like this:

@dispatch()
selectTab(tab) {
 return this.active.id !== tab.id ? { type: .... } : null;
}

[SethDavenport]

If you're concerned that this results in hard-do-debug behaviour in the case where you forget a return statement then we could do a variant of this where we only skip the dispatch call if the function returns false rather than for any falsy value including undefined.

[SethDavenport]

I agree with you on the returning undefined point. As for 'going against redux' - you are right to ask what "principle" it goes against because actually I don't think that's an issue here personally.

I guess what I'm saying is that both you and I are at a high level proposing a similar solution: return a 'special' value - either false or a special no-op action.

In which case false seems simpler. You still have to explicitly return it for the dispatch not to take place. If you forget to return, it will attempt to dispatch undefined and you'll still get an error.

[rart]

I'm happy with that (returning false and triple equal checking it). If everyone is happy, let me know if the change will make it through and I'll update the PR.

[smithad15]

I don't generally use the decorators due to testing and architectural patterns that I subscribe to, but returning false in order to short-circuit the dispatcher seems to make sense from an API perspective. It also reduces the performance overhead of dispatching another action, just so you don't dispatch an action :)

[e-schultz]

@rart your example of

@dispatch()
selectTab(tab) {
  return (this.active.id !== tab.id) 
    ? { type: .... } 
    : false
}

Gives me a better sense of a use-case where your idea can help out.

I'm more inline with the idea of not dispatching falsy values (null, undefined, false) than using a special no-op action type, then leaving the action upto the decorator to either dispatch or do nothing.

[e-schultz]

As discussed in the PR - I think having this be checking for undefined, or a falsy value instead of a special action-type/magic string would be a better fit.

Leaving it at the decorator level like this is appropriate and I like it (instead of trying to change the actual store.dispatch), as these decorators/helpers are more geared towards smoothing out use with Angular and going a step beyond providing a binding between Angular and Redux.

I recently updated master tweaking some of the style / formatting rules to use prettier - so you may need to rebase and run the formatter also.

[e-schultz]

Since this check is going into the @dispatch decorator, it might never actually hit the redux dispatcher, I'm in favour of the idea of being able to short-circuit at that level.

If this was trying to change store.dispatch - then I'd be inline of "starts to go against redux" to speak to @danielfigueiredo point.

Also agree with @smithad15 in not wanting to have 'noop' action types flying around - creates noise in logs / etc, potential performance hits with reducers/selectors/going through middleware/etc - even if only minor, it's still noisy.

I'm ok with moving forward with this, thanks for the contribution and feedback.

[danielfigueiredo]

Sorry, I didn't mean bad with going against Redux. My thinking was just that @dispatch performs a dispatch, and doing something different than what Redux does when invalid actions are passed in might confuse a bit, since the APIs would be different. That said, if returning undefined or null is something you guys are cool with, awesome! Here's another option that could help brainstorming :)

@dispatch could take an Function as an argument that evaluates whether it should dispatch or not. The current condition is the default value, pseudo code could be something like:

type DispatcherFilter = <A extends Action, RootState>(action: A, store: Store<RootState>) => boolean;
const shouldDispatch: DispatcherFilter = (action: Action, store: Store<any>) => !!store;

export function dispatch(shouldDispatchAction: DispatcherFilter = shouldDispatch): PropertyDecorator {
  return function decorate(
    target: Object,
    key: string | symbol | number,
    descriptor?: PropertyDescriptor): PropertyDescriptor {

    let originalMethod: Function;

    const wrapped = function (this: any, ...args: any[]) {
    const result = originalMethod.apply(this, args);
    if (shouldDispatchAction(result, store)) {
      store.dispatch(result);
    }
    return result;
  }

[e-schultz]

@rart been busy with the holidays, but will try and get this wrapped up in the next day or two. Sorry for the delays.

[rart]

NOTE there seems to be a new hook that runs before you commit. I had a single quoted string like 'shouldn\'t call dispatch.' and the hook was silently changing my single quotes to double quotes, making the ci tool that runs here on GitHub fail the tests. I got rid of the escaped single quote inside the string and then it all worked: beware of this behaviour!

[rart]

Adjusted the @dispatch decorator: dismissed the NO_DISPATCH constant and made the decorator check for result === false in order to not dispatch.

As we discussed earlier in this thread, I used false instead of falsy values to avoid silent failures of developers forgetting to return instead of purposefully wanting to avoid dispatch.

[e-schultz]

This is in v7.1.0 now which just got released, thanks!

[rart]

Sweet, thank you!