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

Extending pipes cause template parse errors #8694

Closed
mramos-dev opened this issue May 17, 2016 · 23 comments
Closed

Extending pipes cause template parse errors #8694

mramos-dev opened this issue May 17, 2016 · 23 comments
Assignees
Labels

Comments

@mramos-dev
Copy link

mramos-dev commented May 17, 2016

IMPORTANT: This repository's issues are reserved for feature requests and bug reports. Do not submit support requests here, see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question.

Steps to reproduce and a minimal demo of the problem

Visit this plunker and bring up the developer tools to see the errors being logged to the console:
http://plnkr.co/edit/HKRTePVCqumMGAeqeyPj?p=preview

I have a CustomDataPipe defined that extends the DatePipe. When updating the rc1, I started receiving console errors about not being able to find the custom pipe. This was working in 2.0.0-beta.17.

Current behavior
Extending pipes are causing template parse errors.

Expected/desired behavior
Should be allowed to extend pipes.

Other information

Update 2016.10.03

See http://plnkr.co/edit/YNLNieCDw8Qb90TsovQr?p=preview from @chrisnicola

The problem is that the cad pipe override the definition of CurrencyPipe

  • cad Pipe is not found,
  • if replaced we currency in the plunker, the cad pipe is used,
@mhevery mhevery self-assigned this May 17, 2016
@zoechi
Copy link
Contributor

zoechi commented May 23, 2016

The relevant files in the plunker are

  • app/custom-date.pipe.ts
  • app/hero-birthday1.component.ts

@conartist6
Copy link

conartist6 commented May 24, 2016

I'm also experiencing this issue. I have an stranslate pipe class derived from a translate pipe class.
stranslate's constructor should receive an additional parameter, however it is injected in the template with the constructor parameters annotated by its parent.

@conartist6
Copy link

conartist6 commented May 24, 2016

More information, as I start to think it might become relevant. I'm using ES5.

I think I've found out more or less what's going on. While we're inside metadata_resolver's getPipeMetadata, we call into getTypeMetadata, which calls into the reflection system. The reflection system runs the following two lines:

            var paramAnnotations = this._reflect.getMetadata('parameters', typeOrFunc);
            var paramTypes = this._reflect.getMetadata('design:paramtypes', typeOrFunc);

The first line works fine, paramAnnotations is correct. The reflector finds that metadata exists for the property 'parameters' for typeOrFunc (my subclass constructor). It returns an array of three arrays, each length one containing a constructor, one for each class which should have an instance injected.

The second line is unable to find metadata for my constructor, but does find metadata for its parent.

I don't yet understand why this step is being done. paramAnnotations[1][0] === paramTypes[0] //true, so it isn't actually gaining data it didn't have before (in this instance).

Then the types and the the annotations are "zipped" and the non-matching data is discarded, rendering the state internally consistent even though it wasn't.

@conartist6
Copy link

I was able to find a workaround. Ugly but it works.

var ScopedTranslatePipe = Pipe({
        name: 'stranslate'
    }).Class({
        extends: TranslatePipe,
        constructor: [ModuleScope, TranslateService, ChangeDetectorRef, function(scope, translate, _ref) {
            TranslatePipe.call(this, translate, _ref);

            this.scope = scope;
        }]
    });
Reflect.defineMetadata('design:paramtypes', [ModuleScope, TranslateService, ChangeDetectorRef], ScopedTranslatePipe);

@conartist6
Copy link

I'm now confident that the root cause of my issue is extending an es6 pipe using es5 annotation methods.

@lacolaco
Copy link
Contributor

lacolaco commented Sep 6, 2016

@mhevery any activities for this problem?

@vicb
Copy link
Contributor

vicb commented Sep 6, 2016

Could some quickly describe:

  • expected behavior,
  • current behavior,
  • steps to reproduce

@conartist6
Copy link

conartist6 commented Sep 6, 2016

Expected behavior:
Extending a pipe class defined in typescript using es5 syntax and adding a constructor parameter doesn't error out.

Current behavior (??? could be fixed by now I'm not using A2 at the moment and the original plunk seems to be broken): The code will error out because the child class, with different constructor parameters, will be injected with types based on the parent constructor's signature.

Steps to reproduce: In a working app, define a pipe using

var ProblemPipe = Pipe({ name: 'problemPipe', pure: false }).Class({
    extends: AsyncPipe,
    constructor: [MyParamType, ChangeDetectorRef, function(_myParam, _ref) {
        AsyncPipe.call(this, _ref);
    }]
});

The parent class constructor is defined as
constructor(_ref: ChangeDetectorRef) { this._ref = _ref; }

Its constructor injection information is used, and a single ChangeDetectorRef is injected into ProblemPipe and assigned to _myParam. Pipe cannot be constructed because it is passed null.

@vicb
Copy link
Contributor

vicb commented Sep 6, 2016

thanks for the details.
any plunker based on the latest RC6 ?

@conartist6
Copy link

Not at present.

@vicb vicb added needs reproduction This issue needs a reproduction in order for the team to investigate further need: repro labels Sep 6, 2016
@lacolaco
Copy link
Contributor

lacolaco commented Sep 7, 2016

I created a repro. http://plnkr.co/edit/zW5eP9oxI3e3pDwwNsFv
I'm not sure to whether this really matches to this issue.

@conartist6
Copy link

Thanks! I've edited your repro to what I believe should show the issue, but for some reason this code is failing before it gets to the injection phase. Maybe you know what's going on?

http://plnkr.co/edit/yScThy1F2vUSvgJWufiP

@conartist6
Copy link

OK I've updated that plunk further to the point where it is clear that the issue as described is no longer occurring. For some reason my class which extends AsyncPipe isn't working, but the extended class is certainly being injected correctly.

@conartist6
Copy link

Ah, I needed to make it pure since the new Pipe metadata overwrites the original.

@conartist6
Copy link

Close it up!

@mhevery mhevery added area: core Issues related to the framework runtime and removed comp: core labels Sep 7, 2016
@conartist6
Copy link

I also just looked back at the original plunk by the bug creator. His issue isn't quite the same as the one I was having, but I think I suspect his problem was, like mine at the very end, that he should have added pure to his pipe's metadata.

@zoechi
Copy link
Contributor

zoechi commented Sep 8, 2016

@conartist6 I don't think a template parse error is related to pure: true/false ;-)

@chrisnicola
Copy link

Yeah I've tested the repro on the plunker with pure: true and pure: false this is definitely a major problem. It is essentially not possible to extend the core pipes as far as I can tell.

@chrisnicola
Copy link

Here is an updated repro : http://plnkr.co/edit/YNLNieCDw8Qb90TsovQr?p=preview

@vicb let me know if I should just open a new issue for this

@vicb vicb added type: bug/fix and removed need: repro needs reproduction This issue needs a reproduction in order for the team to investigate further labels Oct 4, 2016
@vicb vicb self-assigned this Oct 4, 2016
@vicb
Copy link
Contributor

vicb commented Oct 4, 2016

@chrisnicola thanks - I'll update the description with your plunker & some analysis

@vicb
Copy link
Contributor

vicb commented Oct 4, 2016

relates to #11606

@DzmitryShylovich
Copy link
Contributor

#11606 was merged so this can be closed @pkozlowski-opensource

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

No branches or pull requests

8 participants