Skip to content

PipeTransform docs unclear when dealing with multiple parameters #37321

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
mdelez opened this issue May 28, 2020 · 9 comments
Closed

PipeTransform docs unclear when dealing with multiple parameters #37321

mdelez opened this issue May 28, 2020 · 9 comments

Comments

@mdelez
Copy link

mdelez commented May 28, 2020

📚 Docs or angular.io bug report

Description

The PipeTransform's transform parameters are unclear as it can now support multiple individual parameters or a rest param. It should be clarified how you can pass multiple parameters to the transform method. I suggest adding an additional example with multiple parameters as both of these ways of invoking the method are valid:

export class MyPipe implements PipeTransform { 
    // specify every argument individually   
    transform(value: any, arg1: any, arg2: any, arg3: any): any { }
    // or use a rest parameter
    transform(value: any, ...args: any[]): any { }
}

Example addition suggestion:

@Pipe({
    name: 'truncate'
})
export class TruncatePipe implements PipeTransform {

    transform(value: string, limit: number, trail: string): string {
        return value.substring(0, limit) + trail;
    }

}

Invoking {{ 'Truncate me' | truncate:8:'!' }} in a template produces Truncate!.

Additionally, it would be nice to document how to write a unit test for a pipe with multiple parameters. Similar to this:

it('should support multiple pipe parameters in the template', () => {
        @Component({
          template: `{{ text | truncate:8:'...' }}`,
        })
        class App {
          text = 'Truncate me';
        }

        TestBed.configureTestingModule({declarations: [App, TruncatePipe]});
        const fixture = TestBed.createComponent(App);
        fixture.detectChanges();

        expect(fixture.nativeElement.textContent).toEqual('Truncate...');
    });

What's the affected URL?**

https://angular.io/api/core/PipeTransform

📷Screenshot

Screen Shot 2020-05-28 at 11 57 35

Screen Shot 2020-05-28 at 12 09 19

Can I open up a PR to make this change?

@ngbot ngbot bot added this to the needsTriage milestone May 28, 2020
@Airblader
Copy link
Contributor

A unit test for a pipe generally doesn't need a component or a TestBed at all. You can simply instantiate the pipe and call the transform method.

I'm not sure I understand the issue you're describing. Are you saying that it's unclear from the interface definition they a concrete implementation can specify an explicit list of parameters? That falls into the category "that is how TS works and doesn't relate to Angular", IMO. Showing an example would probably be fine but the Angular docs don't need to teach TS as well in my opinion.

@mdelez
Copy link
Author

mdelez commented May 29, 2020

Are you saying that it's unclear from the interface definition they a concrete implementation can specify an explicit list of parameters? That falls into the category "that is how TS works and doesn't relate to Angular", IMO.

Yeah this is basically what I'm saying. I think just adding a second example which uses multiple parameters would be nice.

@mdelez
Copy link
Author

mdelez commented Jun 4, 2020

@Airblader also, just curious why Angular themselves would write a unit test for a pipe using a component. This is where I got the idea.

it('should support arguments in pipes', () => {
@Component({
template: `{{person.name | multiArgPipe:'one':person.address.city}}`,
})
class App {
person = {name: 'value', address: {city: 'two'}};
}
TestBed.configureTestingModule({declarations: [App, MultiArgPipe]});
const fixture = TestBed.createComponent(App);
fixture.detectChanges();
expect(fixture.nativeElement.textContent).toBe('value one two default');
});

@Airblader
Copy link
Contributor

These are framework tests, so you want to make sure the pipe as a mechanism works end to end. When writing your own pipe you don't need to test that the framework works, though.

Note that there certainly are pipes where a component unit test can be useful, just the majority of pipes in my experience wouldn't need it.

@tobiasschweizer
Copy link

@Airblader

Are you saying that it's unclear from the interface definition they a concrete implementation can specify an explicit list of parameters? That falls into the category "that is how TS works and doesn't relate to Angular", IMO. Showing an example would probably be fine but the Angular docs don't need to teach TS as well in my opinion.

I think you are right that the Angular docs cannot teach people TypeScript.

But how multiple parameters are passed to a pipe from the template is clearly an Angular related question.

The docs state:

A pipe can accept any number of optional parameters to fine-tune its output. To add parameters to a pipe, follow the pipe name with a colon ( : ) and then the parameter value (such as currency:'EUR'). If the pipe accepts multiple parameters, separate the values with colons (such as slice:1:5)
https://angular.io/guide/pipes#parameterizing-a-pipe

And I think it would be nice to show the implementation of a pipe that takes multiple parameters. (I found the ExponentialStrengthPipe example in the docs with an optional param, why not add it to that section?)

We would be happy to provide such an example. Otherwise please feel free to close this issue.

@Airblader
Copy link
Contributor

But how multiple parameters are passed to a pipe from the template is clearly an Angular related question.

Definitely agreed.

@matthewjh
Copy link
Contributor

matthewjh commented Jun 9, 2020

A unit test for a pipe generally doesn't need a component or a TestBed at all. You can simply instantiate the pipe and call the transform method.

You can do, yes, although then you are reducing the level of assurance and coverage the test gives (for example, testing the name of the pipe, pure vs impure pipes), kind of like testing a component through the TestBed and DOM versus instantiating a component directly and calling methods on the instance. If someone changes a key attribute of the pipe in a codebase, such as its metadata, you want to have tests that cover that to prevent breakages.

What we do in our codebase is have a pipe-used-in-component-template test, to validate the name and other attributes that would be missed by calling transform, but for the bulk of the input versus output test cases, call transform directly, mostly for performance reasons. If it was possible to use the former method as quickly, we would happily forgo the latter.

In general, I think it's best to test both pipes and components as they are used in the real-world.

Anyway, I agree with @mdelez that the doc is not very clear. Clarity that pipes can be written to take 0..N arguments, as well as how these can be passed from component templates, would be helpful.

@cesperian
Copy link
Contributor

@aikidave can you please assign this one to me? Also, there is an open PR now for it, so please add has PR label also? Thanks!...

@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 Mar 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants