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

Feature: Introduce *call* pipe #20419

Closed
dawidgarus opened this issue Nov 14, 2017 · 27 comments
Closed

Feature: Introduce *call* pipe #20419

dawidgarus opened this issue Nov 14, 2017 · 27 comments
Labels
area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime core: change detection feature: under consideration Feature request for which voting has completed and the request is now under consideration feature: votes required Feature request which is currently still in the voting phase feature Issue that requests a new feature
Milestone

Comments

@dawidgarus
Copy link

dawidgarus commented Nov 14, 2017

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

Using function in template like this {{func(arg1, arg2)}} would cause function call on every change detection, even when the arguments didn't change.
One could write pure pipe to avoid it, but it's a tedious job when You have a lot of relatively simple functions that are used only in one component and it pollutes module namespace.

Expected behavior

call pipe should be introduced, which would call a function with arguments, like this: {{func | call:this:arg1:arg2}}. call should be pure, so it would be called only when an argument changes.

What is the motivation / use case for changing the behavior?

Less code without unnecessary pipes created to be used only once or in one component.

@jasonaden jasonaden added area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime labels Nov 14, 2017
@vicb vicb added the feature Issue that requests a new feature label Nov 21, 2017
@vicb
Copy link
Contributor

vicb commented Nov 21, 2017

It would mean that your template has to know about your implementation details.
It's probably a better idea to implement this on the TS side.

@dawidgarus
Copy link
Author

@vicb Why would You need to know implementation details? All you need to know is that there is a function func with arguments arg1 and arg2.
To implement this on TS side, you would have to write something like this:

class MyComponent {
	arg1: any;
	arg2: any;
	funcResult: any;
	
	private arg1Copy: any;
	private arg2Copy: any;

	ngDoCheck(): void {
		if (this.arg1 !== this.arg1Copy || this.arg2 !== this.arg2Copy) {
			this.arg1Copy = this.arg1;
			this.arg2Copy = this.arg2;
			this.funcResult = this.func(arg1, arg2);
		}
	}
}

And that's just for one function call. What if the were more? Observables really don't make it easier, unless you already have observables for arg1 and arg2.
Also, try to imagine that You wan't to call this function inside NgFor, for example:

<div *ngFor="let item of list">{{func(item, arg2)}}</div>

@nayfin
Copy link

nayfin commented Sep 25, 2018

this makes a lot of sense for component logic that needs to be run in the template and in the component class.

component

@Component({...})
export class SomeComponent implements OnInit {
    numbers = [1,2,3,4,5,6];
    
    ngOnInit() {
         numbers.filter(isEven).forEach( console.log)
    }

    isEven(value: number) {
        return value % 2 === 0;
    }
}

template

<ul>
    <li *ngFor="let item of numbers" [blue-background]="item | callback : isEven"></li>
</ul>

This is a trivial example, but the alternative is to create many more specialized pipes, Then either keep the logic in a service that both the component and the pipe ingest, or import the pipe and run its logic with .transform. Sometimes components have unique logic that doesn't need to be abstracted into a service and forcing it into a service or a pipe just makes unnecessary abstraction. It a little weird until you know about it, but this wouldn't be the first instance of that in Angular ;)

@nayfin
Copy link

nayfin commented Sep 27, 2018

I fleshed out a stackblitz to try and demonstrate the utility and performance benefits of this implementation.

I would also like to point to the end of an otherwise very good article on Angular best practices, where the author advocates for functions in the template. He demonstrates in other articles how using pipes is a better option, but a lot of beginning courses teach this method as well, then later the dev learns that it's bad practice. This pipe could lower the learning curve on implementing functions in templates in a performant manner.

@yusijs
Copy link

yusijs commented Sep 27, 2018

A more dynamic approach would be to use spread on args, which will allow the consumer to use any functions with a dynamic set of arguments

  transform(fn: Function, ...args): any {
    return fn(...args);
  }

template:

<span>{{ aFunction | callback:arg1:arg2:arg3:arg4 }}</span>

ts:

public aFunction(arg1, arg2, arg3, arg4) {
  ...
}
// or
public aFunction(args) {
  const arg1 = args[0], arg2 = args[1], ....
}

Not sure if I think this should go into the library though - it's very simple to implement this yourself, and the stack traces are easier to debug if you have everything in your own code (imo).

@mlc-mlapis
Copy link
Contributor

@yusijs ... yep, easy enough but the problem is that a lot of Angular devs today still don't see/understand/know about this way and why it is so useful, so adding this as a core feature can help them a lot.

@dawidgarus
Copy link
Author

As an alternative approach, new feature could be introduced: @PipeFunction

class MyComponent {
  @PipeFunction()
  myPipe(value, ...args) { // method must be compatible with PipeTransform.transform
    // do something
  }
}

it could be used in template as regular pipe: {{ value | myPipe }} but it would be available only in MyComponent's template.
But this feature would probably require changes in the compiler.

@mlc-mlapis
Copy link
Contributor

@dawidgarus ... I like the name @Pure instead of @PipeFunction.

@skreborn
Copy link
Contributor

@mlc-mlapis If the compiler has to be changed either way, wouldn't it be better to keep the normal syntax for functions and just mark them @Pure? Perhaps this would be sufficient for the compiler to know that it has to mimic a pure pipe and track changes to the input values, but I'd really love to keep the call syntax (and the code-completion that comes with it).

export class MyComponent {
  public foo = 1;
  public bar = 4;

  @Pure()
  public add(a: number, b: number): number {
    return a + b;
  }
}
<span>{{ add(foo, bar) }}</span>

@trotyl
Copy link
Contributor

trotyl commented Sep 27, 2018

@skreborn Mostly duplicate of #20472 in that way.

@dawidgarus
Copy link
Author

@trotyl computed properties can't do this: <div *ngFor="let item of list">{{func(item, arg2)}}</div>

@skreborn
Copy link
Contributor

skreborn commented Sep 27, 2018

@trotyl You could say that, but while the end result is mostly the same, it's different in a way that arbitrary properties aren't tracked, only the input values - and that mechanism is already implemented for pipes.

For example, in the following scenario, the @Pure function wouldn't update its value when baz changes, only when either foo or bar does.

export class MyComponent {
  public foo = 1;
  public bar = 4;

  @Pure()
  public add(a: number, b: number): number {
    return a + b + this.baz;
  }

  private baz = 9;
}
<span>{{ add(foo, bar) }}</span>

@mlc-mlapis
Copy link
Contributor

mlc-mlapis commented Sep 27, 2018

@skreborn ... you described exactly what I intended by that @Pure decorator. 👍 🏅

@trotyl
Copy link
Contributor

trotyl commented Sep 27, 2018

computed properties can't do this

@dawidgarus With observed computed properties, there could be different paradigm like:

<div *ngFor="let item of items; index as i">{{ items[i] }}
class MyComponent {
  @Input() list: any[]

  @Computed()
  get items() {
    return this.list.map((item) => this.func(item, arg2))
  }
}

I agree it not really duplicate for the preference difference, but would both achieve the goal of being performant.

it's different in a way that arbitrary properties aren't tracked

@skreborn The @Pure() function would not track them as well in this proposal if accessed via this, and technically computed property could support other tracked source other than @Input(), eg. @Observe(), resulting to:

export class MyComponent {
  @Observe() foo = 1;
  @Observe() bar = 4;

  @Computed()
  get sum(): number {
    return this.a + this.b + this.baz;
  }

  private baz = 9;
}

@skreborn
Copy link
Contributor

skreborn commented Sep 27, 2018

@trotyl That is exactly the intention. If the values are accessed via this, they aren't tracked, only the input parameters are, effectively making a @Pure function independent of the class (and act much like a regular pipe), but still usable only locally. It does indeed have access to the class instance as a member function, but it cares not for its changes, only about the input parameter changes within the template, which could come from anywhere (again, just as a pipe would behave).

@dawidgarus
Copy link
Author

dawidgarus commented Sep 27, 2018

@trotyl It's still different functionality which forces you to make conceptual changes in code and fell like a weird workaround. For every example you give with computed properties I can give another how it's different. How about: <ng-container *ngFor="let item of items$ | async"><div *ngIf="getData(item) | async as data">{{func(data, arg2)}}</div></ng-container>

@nayfin
Copy link

nayfin commented Sep 27, 2018

@yusijs the implementation of introducing arguments is arbitrary. I realize spreading an array is the "Angular way" but I personally don't like to marry function parameters to indices in an array, I think a config object of key values is cleaner. Though, I don't really care how arguments are passed.

Additionally, it being easy to implement has nothing to do with it. The json pipe is easy to implement, as are the text casing pipes and the slice pipe but they are all core pipes because they are likely to be helpful and reduce boilerplate. One of the main reasons already stated is that it will simplify the transition from away from functions in templates for new Angular users, for whom creating this pipe won't be as straightforward. This would also reduce the need to make specialized one off pipes that are only used in a single component, and simplify using the same logic in the template and in the component when necessary, which occurs regularly.

Also, your implementation is different from my proposal. You are piping the function and passing arguments. I am piping the value into a function which is the first argument of the callback pipe, additional arguments can follow. These should always be pure functions that don't manipulate the component values.

I would be happy with the @Pure decorator as well if it will give a cleaner implementation of the same functionality. The pipe does seem like it could be integrated much more quickly though.

@trotyl
Copy link
Contributor

trotyl commented Sep 27, 2018

@dawidgarus Then you'll have something like:

<ng-container *ngFor="let item of items">
  <div *ngIf="item[0] | async">{{ item[1] | async }}</div>
</ng-container>
class MyComponent {
  @Input() list: any[]

  @Computed()
  get items() {
    return this.list.map((item) => {
      const tmp = this.getData(item).pipe(publish())
      return [tmp, tmp.pipe(map(_ => this.func(_, arg2)))
    })
  }
}

Anything can be done in template can be done in logic, it’s basic theory, but template could save more and more time when goes complicated.

@dawidgarus
Copy link
Author

dawidgarus commented Sep 27, 2018

Anything can be done in template can be done in logic, it’s basic theory, but template could save more and more time when goes complicated.

Exactly. It get's complicated because it's different feature. It's like using a cow to mow the lawn - you can do it, but that's not the purpose of the cow. Imagine doing code review of your snippet and you are like: who wrote this little monster?

@nayfin
Copy link

nayfin commented Oct 16, 2018

here is a stackblitz with a bunch of different implementations and comparisons of functions directly in template vs pure pipe vs impure pipe. I have finished about 80 percent of the code for a PR if the teams sees the need for it.

Users can easily watch one or multiple values for changes or pass multiple arguments to pipe. I would like to hear what others have to say about pure: true vs pure: false. A pure pipe would lose change detection on referenced values (arrays, objects) but would be more performant when used with primitives. While an impure pipe would allow us to pass referenced variables to the pipe, but change detection would be triggered on every digest.

I personally think that pure: true is the way to go as primitives should be the primary type of data passed to the pipe, and making it impure kind of defeats the purpose.

@jelbourn I'm sure you're busy but I would love to get something like this into core and am happy to do all the work. I would just like to know how likely it is to be accepted before putting in the time.

EDIT: I should have called out @vicb as he was already on this issue. Does this seem like a viable solution, or are you guys going down the @Pure decorator route? I'm using the callback pipe in my library and I would like to set it up to be able to easily transition if/when similar functionality is implemented in @angular/core. Thanks!

@david-shortman
Copy link

Seems like discussion died down a while ago. Wondering if a pipe like this would be worth introducing as a core pipe:

import { Pipe, PipeTransform } from '@angular/core';

// https://github.com/ArtemLanovyy/ngx-pipe-function
@Pipe({
  name: 'variadicFunctionPipe',
})
export class VariadicFunctionPipe implements PipeTransform {
  public transform<Input extends any[], Output>(
    values: Input,
    handler: (...values: [...Input]) => Output,
    context?: unknown
  ): Output {
    if (context) {
      return handler.call(context, ...values);
    }

    return handler(...values);
  }
}

with usage like

@Component({
   selector: 'demo-component',
   template: `
     {{ [firstName, lastName] | variadicFunctionPipe: fullName }}
   `,
 })
 export class DemoComponent {
   @Input() public firstName = 'David';
   @Input() public lastName = 'Shortman';

   public fullName(firstName: string, lastName: string): string {
     return `${firstName} ${lastName}`;
   }
 }

@petebacondarwin
Copy link
Member

petebacondarwin commented Nov 19, 2020

If I were to take a view, I would say that supporting a @Pure() decorator that can be applied to functions/methods that are used in templates would be the best way to do this.

The CallPipe approach requires the user of the function to know about whether it is pure or not, and also to use a somewhat non-JS syntax: [arg1, arg2] | call : fnName rather than simply fnName(arg1, arg2).
One downside to the @Pure approach is if the original developer of the function did not mark the function as pure in the first place. But then the user of the function could just wrap the function with a new one that is marked pure to workaround this.

Furthermore, IVY already has support for this internally with its pureFunction() instruction. See https://github.com/angular/angular/blob/6d8c73a4d62380bd7cfcaeaa23d5db802467d485/packages/core/src/render3/pure_function.ts. This is used internally where the Angular compiler must generate a function call that it knows to be pure - e.g. where a template contains a literal that has computed properties. I think this could be leveraged in the compiler for functions marked with a @Pure decorator...

@angular-robot angular-robot bot added the feature: votes required Feature request which is currently still in the voting phase label Jun 4, 2021
@angular-robot
Copy link
Contributor

angular-robot bot commented Jun 4, 2021

Just a heads up that we kicked off a community voting process for your feature request. There are 20 days until the voting process ends.

Find more details about Angular's feature request process in our documentation.

@angular-robot angular-robot bot added the feature: under consideration Feature request for which voting has completed and the request is now under consideration label Jun 5, 2021
@nigrosimone
Copy link
Contributor

nigrosimone commented Mar 12, 2022

From https://github.com/nigrosimone/ng-generic-pipe (npm https://www.npmjs.com/package/ng-generic-pipe)

import { ChangeDetectorRef, EmbeddedViewRef, Type } from '@angular/core';
import { Pipe } from '@angular/core';
import { PipeTransform } from '@angular/core';

@Pipe({
  name: 'ngGenericPipe',
  pure: true
})
export class NgGenericPipe implements PipeTransform {
  private context: any;

  constructor(cdRef: ChangeDetectorRef) {
    // retrive component instance (this is a workaround)
    this.context = (cdRef as EmbeddedViewRef<Type<any>>).context;
  }

  public transform(
    headArgument: any,
    fnReference: any,
    ...tailArguments: any[]
  ): any {
    return fnReference.apply(this.context, [headArgument, ...tailArguments]);
  }
}

usage

import { Component } from '@angular/core';
import { Observable, of } from 'rxjs';

@Component({
  selector: 'app-root',
  template: `{{ name  | ngGenericPipe: testFromPipe:"arg1":"arg2"}}`,
})
export class AppComponent {

  public name: string = 'foo';

  testFromPipe(a: string, b: string, c: string): string {
    console.log(a, b, c);
  }
}

@Nutza974

This comment was marked as abuse.

@pkozlowski-opensource
Copy link
Member

Looking into this (and similar) issues it sounds like we are looking for using a pipe as something akin to computed properties. We do consider a more generic solution. Since we are getting multiple issues / feature requests around computed properties, we've opened a canonical issue #47553 to track this use-case. Currently there is no exact solution / design, but we do acknowledge the use-cases and the need for such concept.

@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 Oct 28, 2022
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 area: core Issues related to the framework runtime core: change detection feature: under consideration Feature request for which voting has completed and the request is now under consideration feature: votes required Feature request which is currently still in the voting phase feature Issue that requests a new feature
Projects
None yet
Development

No branches or pull requests