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

include exception for no-input-rename rule #224

Closed
karptonite opened this issue Jan 27, 2017 · 13 comments
Closed

include exception for no-input-rename rule #224

karptonite opened this issue Jan 27, 2017 · 13 comments

Comments

@karptonite
Copy link

See issue #138.

The style guide has now been amended to include the exception mentioned in that issue:
https://angular.io/docs/ts/latest/guide/style-guide.html#!#05-13

If possible, the linter should catch when the directive selector is the same as the name of the input being renamed, and not flag it in that case.

@mgechev
Copy link
Owner

mgechev commented Jan 27, 2017

Thanks for pointing this out. I'll add it as a low priority enhancement. Before we implement this feature in codelyzer this rule can be selectively disabled.

@ddancziger
Copy link

Hi its my first time and i would love to contribute, understood the issue, can you help me pointing where to start with this?
thanks

@mgechev
Copy link
Owner

mgechev commented Jan 28, 2017

Awesome! In general each TypeScript file is being parsed, by a couple of parsers, which in the end extract the metadata for the components (this includes the one attached with @Component, or @Directive).

There's the so called Ng2Walker class which is responsible for collecting this metadata. If the class is a component the metadata will be an instance of ComponentMetadata, otherwise of DirectiveMetadata. All this happens here.

Each "rule" has a "rule walker" which extends the Ng2Walker (in order to reuse logic). Such walker is InputMetadataWalker which checks if given @Input is being renamed or not. I'd recommend you to override the method visitClassDeclaration in InputMetadataWalker and do something like:

export class InputMetadataWalker extends Ng2Walker {
  metadata: DirectiveMetadata;

  // The override, do not forget to invoke the method from the base class.
  visitClassDeclaration(node: ts.ClassDeclaration) {
    this.metadata = this._metadataReader.read(declaration);
    super.visitClassDeclaration(node);
  }

  visitNg2Input(property:ts.PropertyDeclaration, input:ts.Decorator, args:string[]) {
    let className = (<any>property).parent.name.text;
    let memberName = (<any>property.name).text;

    // check if the selector has the same name as the memberName
    // the selector will be in the form '[selectorName]` so you may need to use
    // compiler from '@angular/compiler', and it's CssSelector.parse export.
    // https://github.com/mgechev/codelyzer/blob/master/src/selectorNameBase.ts#L176
    // After that you'll get "selectorName" as value of any of the elements in the "attrs" property
    // of the returned element.

    // Should be modified.
    if (args.length !== 0 && memberName !== args[0]) {
      let failureConfig:string[] = [className, memberName, memberName];
      failureConfig.unshift(Rule.FAILURE_STRING);
      this.addFailure(
        this.createFailure(
          property.getStart(),
          property.getWidth(),
          sprintf.apply(this, failureConfig)));
    }
  }
}

@ddancziger
Copy link

ddancziger commented Jan 28, 2017

thanks
2 questions

  1. we dont want to mark as fails when ['name'] (Directive Name) and the @input('nameChange') name?
    test:
it('should succeed, when a directive input property is the same as the name of the directive selector name', () => {
      let source = `
      @Directive({
        selector: ['label']
      })
      class ButtonComponent {
        @Input('labelAttribute') label: string;
      }`;
      assertSuccess('no-input-rename', source);
    });
  1. how can i test if it works (firs time in this project) with tests (how can i run them)? or there is another way before making a test? so i can debug
    thanks

@mgechev
Copy link
Owner

mgechev commented Jan 28, 2017

Yes, in the above case it should not fail.

You can run the tests with npm t. Once you're sure everything works fine, you can run npm run build to make sure the declarations are generated properly. During development, you can run:

# Terminal window 1
$ npm run tsc:watch
# Terminal window 2
$ npm run test:watch

This way on change the tests are going to run automatically.

@ddancziger
Copy link

sorry for many questions but im learning a lot
i just wondering how to get the Directive decorator selector inside the visitNg2Input i have the metadata declaration and inside the parent.text i have the info.
how can i get from there the name of the selector ['name']
thanks

@mgechev
Copy link
Owner

mgechev commented Jan 28, 2017

Here's the example from above:

export class InputMetadataWalker extends Ng2Walker {
  metadata: DirectiveMetadata;

  // The override, do not forget to invoke the method from the base class.
  visitClassDeclaration(node: ts.ClassDeclaration) {
    this.metadata = this._metadataReader.read(declaration);
    super.visitClassDeclaration(node);
  }

  visitNg2Input(property:ts.PropertyDeclaration, input:ts.Decorator, args:string[]) {
    let className = (<any>property).parent.name.text;
    let memberName = (<any>property.name).text;

    // check if the selector has the same name as the memberName
    // the selector will be in the form '[selectorName]` so you may need to use
    // compiler from '@angular/compiler', and it's CssSelector.parse export.
    // https://github.com/mgechev/codelyzer/blob/master/src/selectorNameBase.ts#L176
    // After that you'll get "selectorName" as value of any of the elements in the "attrs" property
    // of the returned element.

    // Should be modified.
    if (args.length !== 0 && memberName !== args[0]) {
      let failureConfig:string[] = [className, memberName, memberName];
      failureConfig.unshift(Rule.FAILURE_STRING);
      this.addFailure(
        this.createFailure(
          property.getStart(),
          property.getWidth(),
          sprintf.apply(this, failureConfig)));
    }
  }
}

The directive metadata is being extracted by the MetadataReader in visitClassDeclaration. You can save the metadata for use by the visitNg2Input class.

@ddancziger
Copy link

he thanks for all the help im not beeing able to get done this i write this

export class InputMetadataWalker extends Ng2Walker {
  metadata: DirectiveMetadata;

  visitClassDeclaration(node: ts.ClassDeclaration) {
    this.metadata = this._metadataReader.read(node);
    if (this.metadata instanceof DirectiveMetadata) {

      // let name = this.metadata.decorator.;
      // let directiveClassName:string = name;

      console.log('has a directive', this.metadata.decorator);
    }
    super.visitClassDeclaration(node);
  }

  visitNg2Input(property:ts.PropertyDeclaration, input:ts.Decorator, args:string[]) {
    let className = (<any>property).parent.name.text;
    let memberName = (<any>property.name).text;


    if (args.length !== 0 && memberName !== args[0]) {
      let failureConfig:string[] = [className, memberName, memberName];
      failureConfig.unshift(Rule.FAILURE_STRING);
      this.addFailure(
        this.createFailure(
          property.getStart(),
          property.getWidth(),
          sprintf.apply(this, failureConfig)));
    }
  }
  private extractMainSelector(i:any) {
    return compiler.CssSelector.parse(i.text);
  }
}

i dont understand how to get the directive selector i only get the input
i think its taking me to much time to understand if you think someone else can take it and i will do my next time something more for starter
sorry
and thanks

@mgechev
Copy link
Owner

mgechev commented Jan 29, 2017

You can get the selector with: this.metadata.selector, here's the declaration of the DirectiveMetadata interface.

@ddancziger
Copy link

thank you for your time i managed to pass the test and the only thing its that i need to have the selector like 'label', its not in ['label'] (its not take in it)

@directive({
selector: 'label'
})
class ButtonComponent {
@input('labelAttribute') label: string;
}`;

@mgechev
Copy link
Owner

mgechev commented Jan 29, 2017

Make sure you're parsing the selector with CssSelector.parse because there could be a lot of corner cases of nested selectors.

thank you for your time i managed to pass the test and the only thing its that i need to have the selector like 'label', its not in ['label'] (its not take in it)

@Directive({
 selector: 'label'
})
class ButtonComponent {
 @Input('labelAttribute') label: string;
}

What behavior are you planning to implement for this case?

@ddancziger
Copy link

  1. When i use metadata.selector if its like ['label'] says that is undefined so its an issue i cant found the label selector.
  2. the behavior for that case needs to be the name of the input is equal to the selector name you can rename the input.
@Directive({
 selector: ['label']
})
class ButtonComponent {
 @Input('labelAttribute') label: string;
}

how to use with the CssSelector.parse

@mgechev mgechev added this to the 2.0.2 - Actualism milestone Feb 19, 2017
@mgechev
Copy link
Owner

mgechev commented Mar 7, 2017

@karptonite I'd suggest to keep the rule as it is for now. It can be disabled for a specific line in case an input/output rename is required. The snippet in the style guide is just an example which shows one appropriate case when renaming an input is not a bad idea but it's not exhaustive, and is not related to the name of the selector of the directive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants