store icon indicating copy to clipboard operation
store copied to clipboard

Make share() an option for @select()

Open jack-guy opened this issue 7 years ago • 5 comments

This is a...

  • [x] feature request
  • [ ] bug report
  • [ ] usage question

In Angular, it's common to add the .share() operator to the add of an observable that you will be | async piping in a template. Each of the async pipes adds another subscription, which can have all sorts of unintended consequences.

When using the @select decorator, one is by default not sharing.

@select('foo') foo$: Observable<boolean>;
<button [class.bar]="foo$ | async">A</button>
<button [class.bar]="foo$ | async">B</button>
<!-- two subscriptions -->

So I'm forced to use the lengthier syntax:

this.foo$ = ngRedux.select<boolean>('foo').pipe(share());

It'd be great if this were configurable in @select:

// Second argument indicates sharability
@select('foo', true) foo$: Observable<boolean>;

Though by default I think it should probably be on.

jack-guy avatar Nov 19 '17 00:11 jack-guy

I think it's really good idea. With share operator we can avoid creating multiple subscription: Look this discussion: https://github.com/angular-redux/store/issues/229

SMHFandA avatar Nov 20 '17 07:11 SMHFandA

This seems like a reasonable request.

One way I've handled this before though is being able to use 'let' and having something like:

<something *ngIf="foo$ async | let foo">
   <button [class.bar]="foo$ | async">A</button>
   <button [class.bar]="foo$ | async">B</button>
</something>

That said, I think having share be default behaviour should be a safe thing to add. I currently can't think of a case where this would cause an issue, although if anyone has thoughts on that let me know.

e-schultz avatar Nov 23 '17 16:11 e-schultz

Also, there is @select$ which lets you add on an observable to a selector. Changelog Notes

import { select$ } from 'angular-redux/store';

export const debounceAndTriple = obs$ => obs$
  .debounce(300)
  .map(x => 3 * x);

class Foo {
  @select$(['foo', 'bar'], debounceAndTriple)
  readonly debouncedFooBar$: Observable<number>;
}

Should be able to do something like:

import { select$ } from 'angular-redux/store';

export const share = obs$ => obs$.share()

class Foo {
  @select$(['foo', 'bar'], share)
  readonly sharedFooBar$: Observable<number>;
}

e-schultz avatar Nov 23 '17 16:11 e-schultz

It's my personal opinion. We should not go far from the usual thinking in angular. If the observable in angular does not use the share by default, then we should not do this also. Instead, we must mention in the documentation that we have such a feature.

What do you think?

SMHFandA avatar Nov 24 '17 10:11 SMHFandA

@jack-guy, @SMHFandA: share might result is some peculiar effects for users who are not so experienced with rx (like me) might not expect. (maybe why it's not default in angular) (https://medium.com/@mikesnare/angular-async-pipes-beware-the-share-bcc9c1cd849d) or when using shared observable asynch pipe inside *ngIf and inside inner templates that I have recently stumbled on which described at https://github.com/angular/angular/issues/10165 that can be solved by using shareReplay() instead of share() as mentioned at https://github.com/angular/angular/issues/10165#issuecomment-341951441.

Having subscription reuse as default for @select doesn't seems so intuitive, I would only expect it will actually just select by reading the decorator name, more like @sharedSelect but having it as an options and easily discoverable in README examples would be great.

Maybe having @sharedSelect decorator actually makes sense.

ambientlight avatar Mar 22 '18 09:03 ambientlight