8000 Support assigning expression result to local variable within templates · Issue #2451 · angular/angular · GitHub
[go: up one dir, main page]

Skip to content

Support assigning expression result to local variable within templates #2451

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

Closed
jeffbcross opened this issue Jun 9, 2015 · 26 comments
Closed
Labels
area: common Issues related to APIs in the @angular/common package feature Issue that requests a new feature freq2: medium state: Needs Design

Comments

@jeffbcross
Copy link
Contributor

Particularly for use cases where piped output is bound to in many places within a template, it's beneficial to have a single piped expression. For example, this observable that's piped into a template:

<div>
  <ul>
    <div *ng-if="!(people | async)">
      Loading people
    </div>
    <div *ng-if="people | async">
      <li *ng-for="person of people | async">
        {{person.name}}
      </li>
    </div>
  </ul>
</div>

This would cause three subscriptions to be created for the people observable, which may re-do the work of getting the observable value if the observable is not a multicast observable. This is also problematic if inside of the ng-if that evaluates to true when the observable has emitted a value, there are expressions that assume people | async is not null, such as: {{people | async.length }}. This would raise an exception since the first value returned from the async pipe would be null.

@vsavkin and I put together a prototype directive, which I'm currently using in the http example that supports assigning the result of a piped expression to a local variable within the template:

<div *assign-local="#unwrappedPeople to people | async">
  <ul *ng-if="unwrappedPeople" class="people">
    <li *ng-for="#person of unwrappedPeople">
      hello, {{person.name}}
    </li>
  </ul>
  <span *ng-if="!unwrappedPeople">
    Fetching people...
  </span>
</div>

This creates a single subscription and simplifies the rest of the template. Here is the implementation: https://github.com/angular/angular/blob/master/modules/examples/src/http/assign_local_directive.ts

I'd like to add this directive as a first-class directive in the angular2 module.

@vsavkin, @mhevery, @tbosch do you have any feedback on the implementation?

@jeffbcross jeffbcross added feature Issue that requests a new feature area: common Issues related to APIs in the @angular/common package P4: nice to have effort1: hours state: Needs Design labels Jun 9, 2015
@tbosch
Copy link
Contributor
tbosch commented Jun 10, 2015

Victor added Directive.exportAs recently. With this, you should be able to write an assign-local directive without the * as follows:

@Directive({
  selector: 'assign-local'
  exportAs: 'assign',
  properties: ['value: assignLocal']
})
class AssignLocal {
  value: any;
}
<div [assign-local]="people | async" #unwrapped-people="assign.value">
...
</div>

@mhevery
Copy link
Contributor
mhevery commented Oct 25, 2015

I was under the impression that change detectors would coalesce multiple reads to the same expressions, so having people|async twice should not be an issue.

To be self consistent with the rest of the templates new variables can only be declared on templates hence * or template would have to be present in the template. Not sure if it is worth the trouble given that change detectors can (or should) coalesce.

@dimitrikochnev
Copy link

Local variable assignment could be also very useful in a combination with Firebase.

I've implemented LVA and it works. Here is the plunk - http://plnkr.co/edit/MWrKGahELHsq2kfu03Gh?p=preview. But it doesn't play nicely with a ng-if directive. I understand why it doesn't pass in the Data 2 block: assign-local executes after ng-if and it doesn't know that the view has been cleared by a ng-if directive, and recreates it.

The question is: how it could be done without wrapping ng-if in a parent container?

viz. refactor

<div *assign-local="#data to promise | async">
  <div *ng-if="data">
    Data 1: {{ data }}
  </div>
</div>

to

<div *assign-local="#data to promise | async" *ng-if="data">
  Data 2: {{ data }}
</div> 

Another point is that when I use ng-if and assign-local together on the same element, assign local evaluates expression again after each view creation by ng-if and in real example it would send another network request if custom async works like

transform(value: string, args: string[]): any {
  ... send network request
}

@KiaraGrouwstra
Copy link
Contributor

I could still access the referenced example here, but was having trouble getting that to work in the latest version (error Invalid left-hand side in assignment, though I'll admit I'm confused w.r.t. whether it should now be defined/invoked as assignLocal over assign-local). Anyway, looking forward to this feature as part of ng2 itself.

@primIl
Copy link
primIl commented Apr 11, 2016

+1

@KiaraGrouwstra
Copy link
Contributor
KiaraGrouwstra commented May 5, 2016

@tbosch: could you comment on whether something like that AssignLocal directive would still be possible since your recent commit ditching setLocal in favor of createEmbeddedViewRef? I guess altering locals in an existing view is no longer supported?

Edit to elaborate; I tried implementing a version of the earlier AssignLocal as follows (excuse the lodash-fp):

@Directive({
  selector: '[assignLocal]',
  inputs: ['localVariable: assignLocal'],
})
export class AssignLocal {
  constructor(public viewContainer: ViewContainerRef) {}
  set localVariable(obj) {
    _.each((v, k) => {
      this.viewContainer._element.parentView.context[k] = v;
    })(obj);
  }
}

In a simple use-case, this just writes expressions to the context -- that is, the component: <div [assignLocal]="{ foo: strExpr }">{{ foo }}</div>.

However, trying this same approach within ngFor, as follows, fails: <div *ngFor="let item of arrExpr; idx = index" [assignLocal]="{ foo: item }" >{{ foo }}</div>.

From what I can see, the context written to in the directive here becomes an NgForRow in this case (e.g. NgForRow{$implicit: 'a', index: 0, count: 3, foo: 'a'}). However, AFAICT the context the subsequent {{ foo }} is evaluated in appears distinct -- containing item and idx as expected but not the $implicit, index, or my freshly-written foo variable contained in the NgForRow.

Forgive me if storing within loops is deemed out of scope of the current topic -- we closed #6947 because this seemed relevant enough. Anyway, this naive implementation may already cover simpler use-cases.

@kylecordes
Copy link

I found a need for about the same thing as this issue asks for; I met it by create a variant of "ngFor" and "ngIf" which doesn't add the DOM until the data is available (like IF), but which also capture the value (like FOR). I called it ngWhen, based on the name of a similar feature in some other languages. I got it up and running as part of an early, pre-angularfire2 A2-firebase library I working on. You can see it in action here:

https://github.com/OasisDigital/angular2-firebase-demo/blob/master/app/weather/weatherPanel.ts#L17

I wonder if something similar (perhaps with better naming etc) ought to be "in the box".

@KiaraGrouwstra
Copy link
Contributor

Link to that ngWhen implementation.
Too bad this way using $implicit with a structural directive appears to be the only way now, notably since the limitation of one structural directive per element (iirc) means you're just clogging up your dom for each variable you'd wish to define. It made sense in kylecordes' "ngIf and variable assignment mashed into one" case, but if need just the latter, not so much.
We'll miss you, setLocal. :(

@kylecordes
Copy link

Right, I'm not proposing my ngWhen as an answer to all questions... just as an example of one thing someone could do to work around the lack of any obvious built-in way to capture the result of blah|async.

@pkozlowski-opensource pkozlowski-opensource added area: core Issues related to the framework runtime freq2: medium severity1: confusing and removed area: core Issues related to the framework runtime effort1: hours labels Oct 8, 2016
@nzjoel1234
Copy link

I agree some kind of variable assignment would be very nice.

One work around I have come across is this:

<div *ngFor="let unwrappedPerson of [person | async]">{unwrappedPerson.name}</div>

@vicb
Copy link
Contributor
vicb commented Dec 13, 2016

FYI this has been implemented for ngIf lately by @mhevery. Other directives should follow soon.

See #13297

@vicb vicb closed this as completed Dec 13, 2016
@KiaraGrouwstra
Copy link
Contributor

@vicb any consideration for a version akin to the AssignLocal so the general case would be handled as well?

@vicb
Copy link
Contributor
vicb commented Dec 14, 2016

No, not yet.
Probably something we can consider at a later point according to usage needs.

@KiaraGrouwstra
Copy link
Contributor

Thanks for your quick response. :)

@eleddie
Copy link
eleddie commented Jun 18, 2017

Any update on this?

@code-tree
Copy link
Contributor
code-tree commented Jul 20, 2017

@vicb Could this be reopened? As it seems mainly to be about a general solution rather than tying it down to ngIf and ngFor etc.

It would appear to solve an issue I'm having with binding ngFor to a function result. The only alternative seems to be to create a separate component simply for the sake of storing the value of one expression.

It would also be one way to solve this issue I believe: #2449

@mischkl
Copy link
mischkl commented Sep 18, 2017

This would be really handy for dealing with async expressions without needing an ngIf or ngFor. For example the Angular 4 release blog post gives the following example:

<div *ngIf="userList | async as users; else loading">
  <user-profile *ngFor="let user of users; count as count; index as i" [user]="user">
User {{i}} of {{count}}
  </user-profile>
</div>
<ng-template #loading>Loading...</ng-template>

What if I need to reference users in multiple places, but I don't need an ng-if wrapper?

Possible solutions currently available:

  1. @mhevery mentions above that the change detectors should coalesce around the same expressions, however accordin 8000 g to Stack Overflow this is not the case after all. (The official documentation doesn't mention how the case of multiple asyncs is handled IMHO - does anyone care to clarify?) The suggested solution is to add a .share() to the observable; however it seems rather error-prone to assume developers will know to do this.

  2. Assuming the developer isn't aware of the .share() trick, and they want to avoid multiple asyncs on the same observable, they will probably be tempted to use subscriptions in the component. This is inelegant and error-prone due to increased boilerplate and the onus of unsubscribing in onDestroy being put on the developer (most probably won't bother and thus open themselves up to memory leaks). To top it off, saving the returned value to an internal property doesn't work in conjunction with OnPush change detection. In my current project, I started off using lots of subscriptions and the resulting code has become pretty unmanageable and in need of refactoring. So IMHO this can be considered a "worst-practice".

  3. There's always the option of breaking things up it up into two components - one that takes an input of a plain object, the other that acts as a wrapper and feeds the first component with a single async pipe. But this seems needlessly complex and creates extra components and DOM where it isn't necessary.

In summary, the most obvious option - subscriptions - is also the worst for a number of reasons. The component-wrapper option is a pain to implement, and the .share() option is non-obvious and not well-documented. It seems to me like it would still be useful to have a general way of assigning asyc expressions to variables within a template, so that there is an obvious, easy-to-use and non-error-prone way to re-use observable data within a template.

I imagine a syntax like the following would be sufficient. I call the directive "ngLet" rather than "assignLocal" as it seems to me more in keeping with Angular template terminology.

<div *ngLet="userList | async as users">
  <user-profile *ngFor="let user of users; count as count; index as i" [user]="user">
User {{i}} of {{count}}
  </user-profile>
</div>

@ericeslinger
Copy link

Yeah, as an aside, I've been using ngIf assignments with my async data structures, which is fine most of the time. I just now ran into a pretty simple situation that makes this workaround annoying though: I'm trying to observe a boolean that represents state in a toggle. So I can't actually do

<div *ngIf="favorited$ | async as favorited">
  <ion-icon name="{{ favorited ? 'star' : 'star-outline'}}">
</div>

since I want it to display even when the item is false. I'm working around by using ngElse directives (with the ion-icon fully expanded) instead of the ternary operator.

I mean, ultimately I could just throw a ton of | asyncs on the page, but it just feels a bit like ngLet or whatever would be a nice addition.

@misaizdaleka
Copy link

Couldn't you do:

<div>
  <ion-icon name="{{ (favorited$ | async) ? 'star' : 'star-outline'}}">
</div>

???

@ericeslinger
Copy link
ericeslinger commented Sep 29, 2017

That works for inline stuff, but not when I'm doing something like toggling a value. How would I do

<button (click)="toggleFavorite(favorited$ | async, $event)">toggle</button>
<!-- error there, you can't put a pipe in an event call signature here -->

I can either do an ngIf like this:

<button *ngIf="favorited$ | async; else notFaved" (click)="toggleFavorite(true, $event)">toggle</button>
<ng-template #notFaved>
  <button (click)="toggleFavorite(false, $event)">toggle</button>
</ng-template>

effectively hardcoding the cu 8000 rrent state into the html element. This works okay, but is kind of brittle - if I need to do other ngIf conditions (say, the button only renders in certain screen sizes), then the if-else thing does not help, because I'm actually combining view logic with state information, which is bound to be brittle.

I can instead write some stuff in my controller:

class Controller implements OnDestroy {
  _toggleClicked = new Subject<MouseEvent>();
  toggleClicked = this._toggleClicked.asObservable();
  toggleFavorite(e: MouseEvent) {
    this._toggleClicked.next(e)
  }
  constructor() {
    this.item$ = item.asObservable() // assume an observable data thing here
    this.toggleWatcher = Observable.combineLatest(this.item$, this.toggleClicked)
      .distinctUntilChanged((o, n) => o[1] === n[1]) // otherwise changes to item will trigger downchain
      .subscribe(([item, evt]) => /* here we know the state of item.favorited and can deal with it */);
  }
  ngOnDestroy() {
    this.toggleWatcher.unsubscribe();
  }
}

This is in fact what I end up doing. There's some extra cruft in that last bit - I usually use a .takeUntil in my in-class subscriptions, and then in ngOnDestroy() I emit on a this.destroying subject I set up, that way if I add more in-class subscriptions I can make sure they all get torn down when the component moves out of scope.

But this is still a fair bit of boilerplate. The nice thing about the async pipe is that it handles the boilerplate of subscribes and unsubscribes as appropriate. There does not seem to be any generally-useful workaround here. Maybe if I dig really far into directives I could write some local binding directive somehow, but this feels like it has a lot of parallels with the ng-model stuff from angular1, which I always felt was rather overused and abused.

For the time being, this is primarily a quality-of-life thing for me. In most scenarios, *ngIf works okay. If I can change the incoming data (from, say, a value to something like {favorited: boolean} which would be truthy even if favorited was false, I can use it even more often). Sometimes I can do inline template expansion with asyncs as @misaizdaleka pointed out. But sometimes I end up having to manually subscribe to a value in my controller, keep an eye on the source of that value (possibly unsubscribing and resubscribing to a new source in ngOnChanges if the component gets recycled (eg., as part of normal ngRouter behavior), and expose that value as a non-async value in the controller so I can get a handle on it in my view.

The alternative, something like

<button [ngLet]="{favorited: ctrl.favorited$ | async}" (click)="dispatch(favorited)">

would sure be rad, saving a ton of boilerplate. Alternatively, putting pipe template expansion in the parameter list of event handler functions would help, but I feel like that's even harder microsyntax.

EDIT to add:

Something simple like this

@Directive({
  selector: '[tLet]',
})
export class Let {
  @HostBinding('attr.data-let')
  @Input('tLet')
  tLet: any;
}

<div [tLet]="ctrl.thing$ | async">
  <button (click)="handle($event)">
</div>

handle($event: MouseEvent & { path: HTMLElement[] }) {
  const val = $event.path.reduce((acc, cur) => acc ? acc : cur.dataset['let'], null);
  console.log(val);
}

is a start. It's not a complete solution - you've got to manually look up the path (which seems to be an out-of-spec thing someone's adding to the MouseEvent) to find where the value was set, and if there are multiple values set, the inner one overrides the outer, and you can only pass strings around (since it gets stringified). I feel like I'm pushing up against the "hey, this is what we designed ngModel for" frontier here.

@AustinMatherne
Copy link
AustinMatherne commented Oct 2, 2017

I wrote up a stripped down version of ngIf which I've been successfully using in a large enterprise app. I'm using the "ng" prefix in the example, but obviously don't do that in your app if you decide to use it.

import {Directive, Input, TemplateRef, ViewContainerRef} from '@angular/core';

interface LetContext<T> {
    ngLet: T;
}

@Directive({
    selector: '[ngLet]'
})
export class LetDirective<T> {
    private _context: LetContext<T> = {ngLet: null};

    constructor(_viewContainer: ViewContainerRef, _templateRef: TemplateRef<LetContext<T>>) {
        _viewContainer.createEmbeddedView(_templateRef, this._context);
    }

    @Input()
    set ngLet(value: T) {
        this._context.ngLet = value;
    }
}
<ng-container *ngLet="selectedUsers$ | async as selectedUsers">
  <app-user-details [users]="selectedUsers"></app-user-details>
</ng-container>

@mischkl
Copy link
mischkl commented Oct 2, 2017

@AustinMatherne awesome! consider doing a mini-pull request to Angular core to fix #15280 and see what happens? :D

@alexzuza
Copy link
Contributor
alexzuza commented Oct 3, 2017

https://stackoverflow.com/questions/38582293/how-to-declare-a-variable-in-a-template-in-angular2/43172992#43172992

@mateja176
Copy link

This is an awesome idea! Is the feature implemented yet, if so can someone please link to the docs?

@johntostring
Copy link

@mytee306
https://angular.io/api/common/NgIf#storing-conditional-result-in-a-variable
https://angular.io/api/common/NgForOf#local-variables

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: common Issues related to APIs in the @angular/common package feature Issue that requests a new feature freq2: medium state: Needs Design
Projects
None yet
Development

No branches or pull requests

0