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

RFC: Use *ngrxLet to create a view model object in the template #3545

Closed
1 of 2 tasks
markostanimirovic opened this issue Aug 23, 2022 · 8 comments · Fixed by #3602
Closed
1 of 2 tasks

RFC: Use *ngrxLet to create a view model object in the template #3545

markostanimirovic opened this issue Aug 23, 2022 · 8 comments · Fixed by #3602

Comments

@markostanimirovic
Copy link
Member

markostanimirovic commented Aug 23, 2022

Information

The *ngrxLet directive could be used to create a view model object in the template as follows:

<ng-container *ngrxLet="{ books: books$, selectedBook: selectedBook$ } as vm">
  <app-book-list [books]="vm.books"></app-book-list>
  <!-- ... -->
</ng-container>

It would be possible to pass a dictionary of observables whose results will be stored in a template variable (vm in this example). If an observable name contains the $ suffix, then the suffix will be removed:

  • books$ => vm.books
  • books => vm.books

EDIT: We decided not to trim the $ suffix from property names to avoid potential naming collisions.

Describe any alternatives/workarounds you're currently using

  1. With *ngIf + async:
<ng-container *ngIf="{ books: books$ | async, selectedBook: selectedBook$ | async } as vm">
  <app-book-list [books]="vm.books!"></app-book-list>
  <!-- ... -->
</ng-container>

The same result can be achieved by using *ngrxLet + ngrxPush.

  1. With combineLatest:
@Component({
  selector: 'app-books',
  template: `
    <ng-container *ngrxLet="vm$ as vm">
      <!-- ... -->
    </ng-container>
  `,
})
export class BooksComponent {
  vm$ = combineLatest({ books: this.books$, selectedBook: this.selectedBook$ });
}

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@e-oz
Copy link
Contributor

e-oz commented Aug 24, 2022

combineLatest is not the same - it will not emit until every source is emitted at least once. It's not an issue when all the sources are from some store, but it might not always be the case.

ngIf map is not quite reliable - in some circumstances, it might ignore the emitted value. I’m from mobile and forgot the exact case - I’ll check later from my laptop. Something related to lifecycle, 100% reproducible.

And the point of my comment is that we absolutely need it because alternatives are not good enough!

@markostanimirovic
Copy link
Member Author

I'd like to see the case when *ngIf map ignores emitted value, thanks @e-oz!

Btw, *ngrxLet with an observable dictionary will have the same behavior as an alternative with combineLatest.

@e-oz
Copy link
Contributor

e-oz commented Aug 24, 2022

Btw, *ngrxLet with an observable dictionary will have the same behavior as an alternative with combineLatest.

Is it possible to make it safe, like

vm$ = combineLatest({ 
    books: this.books$.pipe(startWith(undefined)), 
    selectedBook: this.selectedBook$.pipe(startWith(undefined)),
});

?

Then for streams with values, nothing will be changed - they'll emit their values first (BehaviorSubject, or non-empty ReplaySubject). And values for other streams would be presented as "undefined" (and it's true). This way we can know for sure that vm$ will not be stuck.

@markostanimirovic
Copy link
Member Author

Is it possible to make it safe, like

vm$ = combineLatest({ 
    books: this.books$.pipe(startWith(undefined)), 
    selectedBook: this.selectedBook$.pipe(startWith(undefined)),
});

?

@e-oz In this case, all view model properties need to have | undefined in their type (similar to ngIf + async alternative) if we want a type-safe result.

I'd rather avoid this behavior because the idea for "*ngrxLet + observable dictionary" is to be used with "state" observables (observables that emit the first value synchronously) so we don't need to use the non-null assertion operator in the template for each property even if all passed observables emit the first value synchronously.

If the *ngrxLet directive is used with dictionary that contains async observables, it will render the embedded view when all passed observables emit the first value. Otherwise, if you want to always render embedded view synchronously with *ngrxLet, you would be able to use this type-safe alternative by adding startWith(undefined) to all async observables as follows:

@Component({
  selector: 'app-books',
  template: `
    <ng-container *ngrxLet="{ books: books$, selectedBook: selectedBook$ } as vm">
      <!-- 👉 the type of `vm.books` will be `Book[] | undefined` -->
      <!-- 👉 `selectedBook$` emits synchronously, so the type of `vm.selectedBook` will be `Book` -->
    </ng-container>
  `,
})
export class BooksComponent {
  readonly selectedBook$ = new BehaviorSubject<Book>(defaultBook);
  readonly books$ = this.booksService.getBooks().pipe(startWith(undefined));
}

@e-oz
Copy link
Contributor

e-oz commented Aug 30, 2022

is to be used with "state" observables (observables that emit the first value synchronously) so we don't need to use the non-null assertion operator in the template for each property

Most of my "state" object fields are T|undefined, because I usually have no initial values for them. But maybe it's just me :)

But you are right - there are workarounds.

@edezekiel
Copy link

I really like this idea. A few thoughts that come to mind:

  • This wouldn't be limited to NgRx derived state, right? Could you use it for any dictionary of 'state' observables?
  • Are there advantages/drawbacks to this approach compared to component view model selectors? *ngrxLet seems to save some code over explicit view model selectors.

@markostanimirovic
Copy link
Member Author

Hi @edezekiel,

I'm glad you like it. :)

  • This wouldn't be limited to NgRx derived state, right? Could you use it for any dictionary of 'state' observables?
  • Are there advantages/drawbacks to this approach compared to component view model selectors? *ngrxLet seems to save some code over explicit view model selectors.
  1. Yes, it could be used with any dictionary of 'state' observables.
  2. I also prefer view model selectors/observables, but that's a preference. Using view model selectors is more efficient compared to creating view models via combineLatest. On the other hand, there will be no performance difference when creating view models via the *ngrxLet directive and the combineLatest operator.

@edezekiel
Copy link

Hi @edezekiel,

I'm glad you like it. :)

  • This wouldn't be limited to NgRx derived state, right? Could you use it for any dictionary of 'state' observables?
  • Are there advantages/drawbacks to this approach compared to component view model selectors? *ngrxLet seems to save some code over explicit view model selectors.
  1. Yes, it could be used with any dictionary of 'state' observables.
  2. I also prefer view model selectors/observables, but that's a preference. Using view model selectors is more efficient compared to creating view models via combineLatest. On the other hand, there will be no performance difference when creating view models via the *ngrxLet directive and the combineLatest operator.

Thanks for the response! Based on that I would definitely use ngrxLet except in cases where my component has a facade. When there's a facade I need the typed view model exposed from the facade to the container component.

@markostanimirovic markostanimirovic moved this to In Progress in NgRx v15 Oct 20, 2022
Repository owner moved this from In Progress to Done in NgRx v15 Oct 24, 2022
@markostanimirovic markostanimirovic self-assigned this Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants