mgechev / codelyzer

Static analysis for Angular projects.
http://codelyzer.com/
MIT License
2.45k stars 233 forks source link

overridden BasicTemplateAstVisitor.visitEvent() doesn't visit all events #713

Open etramell opened 6 years ago

etramell commented 6 years ago

What I'm trying to do: Write a custom rule that enforces event handler naming consistency, by copying and modifying the code from banana-in-box.

Problem: The custom rule is not visiting most Angular template events, by which I mean bindings like (event)="handler()".

Minimal example: I've stripped away all the functionality in this rule except for logging the visited event's name (this is the entire TypeScript source file):

import * as ast from '@angular/compiler';
import { NgWalker } from 'codelyzer/angular/ngWalker';
import { BasicTemplateAstVisitor } from 'codelyzer/angular/templates/basicTemplateAstVisitor';
import * as Lint from 'tslint';
import * as ts from 'typescript';

class EventLoggerTemplateVisitor extends BasicTemplateAstVisitor {
  public visitEvent(eventProperty: ast.BoundEventAst, context: BasicTemplateAstVisitor): any {
    console.log(eventProperty.name);

    super.visitEvent(eventProperty, context);
  }
}

export class Rule extends Lint.Rules.AbstractRule {
  public static metadata: Lint.IRuleMetadata = {
    ruleName: 'event-visitor-logger',
    type: 'functionality',
    description: 'Dummy "rule" that just logs all the events it visits.',
    rationale: 'Debugging',
    options: null,
    optionsDescription: 'Not configurable.',
    typescriptOnly: true,
    hasFix: false
  };

  public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
    return this.applyWithWalker(
      new NgWalker(sourceFile, this.getOptions(), {
        templateVisitorCtrl: EventLoggerTemplateVisitor
      })
    );
  }
}

Output:

$ ng lint
click
click
click
click
click
mouseenter
click
submit
click
click
click
click
click
click
click
click
click
click
click
click
click
mouseenter
click
submit
click
click
click
click
click
click
click
click

(Actually, it looks like the events being visited are being visited twice each for whatever reason. Don't know if that's relevant, so I left the duplicate output in there just in case.)

Expected Output: We have many EventEmitter handlers defined in our templates, and none of them show up in this output. Furthermore, our project has 227 (click) handlers defined, and the output above doesn't list anywhere close to that many.

Further troubleshooting: I created another temporary custom rule by copying banana-in-box itself, renaming it to just banana, and adding the same console.log(). I get the same exact output from both of these test rules. In addition, I purposely "put a box inside a banana" and it's not being reported (both banana-in-box and this temporary banana are configured as true).

Versions: Angular 6 and Codelyzer 4.4.2.

  "devDependencies": {
    "@angular-devkit/build-angular": "~0.7.0",
    "@angular/cli": "6.1.5",
    "@angular/compiler-cli": "6.1.7",
    "@angular/language-service": "6.1.7",
    "@angular/platform-server": "6.1.7",
    "@types/jest": "22.2.3",
    "@types/marked": "0.3.0",
    "@types/node": "~6.0.60",
    "@types/pdfjs-dist": "^0.1.2",
    "@types/video.js": "6.2.9",
    "codelyzer": "4.4.2",
    "http-server": "^0.11.1",
    "jasmine-spec-reporter": "~4.2.1",
    "jest": "23.1.0",
    "jest-preset-angular": "5.2.1",
    "ngrx-store-freeze": "^0.2.3",
    "protractor": "~5.1.2",
    "ts-node": "~4.1.0",
    "tslint": "~5.9.1",
    "tslint-consistent-codestyle": "~1.13.1",
    "typescript": "2.9.2",
    "webpack-bundle-analyzer": "^2.11.3"
  }

Any ideas? Thanks!

- Ed

etramell commented 6 years ago

Followup: I have been trying to figure out a commonality between the events that get visited, vs. those that don't, and I think I have it...

In our project, we (very!) often format our HTML like this to avoid long unreadable lines:

<some-component
  [input1]="true"
  [input2]="42"
  (output1)="onOutput1()"
  (output2)="onOutput2()">
</some-component>

instead of:

<some-component [input1]="true" [input2]="42" (output1)="onOutput1()" (output2)="onOutput2()"></some-component>

It appears that the only events that get visited are the ones in HTML like the latter. Events in HTML like the former are not visited.

Given that, is this a bug that can be fixed?

mgechev commented 6 years ago

@etramell-tem thanks for reporting the issue! I'll look at it over the weekend.