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

feat(rule): add new Rule RelativePathExternalResourcesRule #725

Merged
merged 3 commits into from Nov 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions src/index.ts
Expand Up @@ -38,6 +38,7 @@ export { Rule as UseOutputPropertyDecoratorRule } from './useOutputPropertyDecor
export { Rule as UsePipeDecoratorRule } from './usePipeDecoratorRule';
export { Rule as UsePipeTransformInterfaceRule } from './usePipeTransformInterfaceRule';
export { Rule as UseViewEncapsulationRule } from './useViewEncapsulationRule';
export { Rule as RelativePathExternalResourcesRule } from './relativeUrlPrefixRule';

export * from './angular';

Expand Down
66 changes: 66 additions & 0 deletions src/relativeUrlPrefixRule.ts
@@ -0,0 +1,66 @@
import { IOptions, IRuleMetadata, RuleFailure, Rules } from 'tslint/lib';
import { SourceFile } from 'typescript/lib/typescript';
import { NgWalker } from './angular/ngWalker';
import * as ts from 'typescript';

export class Rule extends Rules.AbstractRule {
static readonly metadata: IRuleMetadata = {
description: "The ./ prefix is standard syntax for relative URLs; don't depend on Angular's current ability to do without that prefix.",
descriptionDetails: 'See more at https://angular.io/styleguide#style-05-04.',
rationale: 'A component relative URL requires no change when you move the component files, as long as the files stay together.',
ruleName: 'relative-url-prefix',
options: null,
optionsDescription: 'Not configurable.',
type: 'maintainability',
typescriptOnly: true
};

static readonly FAILURE_STRING = 'The ./ prefix is standard syntax for relative URLs. (https://angular.io/styleguide#style-05-04)';

apply(sourceFile: SourceFile): RuleFailure[] {
return this.applyWithWalker(new RelativePathExternalResourcesRuleWalker(sourceFile, this.getOptions()));
}
}

export class RelativePathExternalResourcesRuleWalker extends NgWalker {
constructor(sourceFile: SourceFile, options: IOptions) {
super(sourceFile, options);
}

visitClassDecorator(decorator: ts.Decorator) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method can be simplified if you use visitNgComponent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using visitNgComponent was my first thought. Unfortunately, I can't access to 'templateUrl' et 'styleUrls'. It seams more efficient with inline template.

if (ts.isCallExpression(decorator.expression) && decorator.expression.arguments) {
decorator.expression.arguments.forEach(arg => {
if (ts.isObjectLiteralExpression(arg) && arg.properties) {
arg.properties.forEach((prop: any) => {
if (prop && prop.name.text === 'templateUrl') {
const url = prop.initializer.text;
this.checkTemplateUrl(url, prop.initializer);
} else if (prop && prop.name.text === 'styleUrls') {
if (prop.initializer.elements.length > 0) {
prop.initializer.elements.forEach(e => {
const url = e.text;
this.checkStyleUrls(e);
});
}
}
});
}
});
}
super.visitClassDecorator(decorator);
}

private checkTemplateUrl(url: string, initializer: ts.StringLiteral) {
if (url && !/^\.\/[^\.\/|\.\.\/]/.test(url)) {
this.addFailureAtNode(initializer, Rule.FAILURE_STRING);
}
}

private checkStyleUrls(token: ts.StringLiteral) {
if (token && token.text) {
if (!/^\.\/[^\.\/|\.\.\/]/.test(token.text)) {
this.addFailureAtNode(token, Rule.FAILURE_STRING);
}
}
}
}
140 changes: 140 additions & 0 deletions test/relativeUrlPrefixRule.spec.ts
@@ -0,0 +1,140 @@
import { Rule } from '../src/relativeUrlPrefixRule';
import { assertAnnotated, assertSuccess } from './testHelper';

const {
metadata: { ruleName }
} = Rule;

describe(ruleName, () => {
describe('styleUrls', () => {
describe('success', () => {
it('should succeed when a relative URL is prefixed by ./', () => {
const source = `
@Component({
styleUrls: ['./foobar.css']
})
class Test {}
`;
assertSuccess(ruleName, source);
});

it('should succeed when all relative URLs is prefixed by ./', () => {
const source = `
@Component({
styleUrls: ['./foo.css', './bar.css', './whatyouwant.css']
})
class Test {}
`;
assertSuccess(ruleName, source);
});
});

describe('failure', () => {
it("should fail when a relative URL isn't prefixed by ./", () => {
const source = `
@Component({
styleUrls: ['foobar.css']
~~~~~~~~~~~~
})
class Test {}
`;
assertAnnotated({
ruleName,
message: Rule.FAILURE_STRING,
source
});
});

it("should fail when a relative URL isn't prefixed by ./", () => {
const source = `
@Component({
styleUrls: ['./../foobar.css']
~~~~~~~~~~~~~~~~~
})
class Test {}
`;
assertAnnotated({
ruleName,
message: Rule.FAILURE_STRING,
source
});
});

it("should fail when one relative URLs isn't prefixed by ./", () => {
const source = `
@Component({
styleUrls: ['./foo.css', 'bar.css', './whatyouwant.css']
~~~~~~~~~
})
class Test {}
`;
assertAnnotated({
ruleName,
message: Rule.FAILURE_STRING,
source
});
});
});
});

describe('templateUrl', () => {
describe('success', () => {
it('should succeed when a relative URL is prefixed by ./', () => {
const source = `
@Component({
templateUrl: './foobar.html'
})
class Test {}
`;
assertSuccess(ruleName, source);
});
});

describe('failure', () => {
it("should succeed when a relative URL isn't prefixed by ./", () => {
const source = `
@Component({
templateUrl: 'foobar.html'
~~~~~~~~~~~~~
})
class Test {}
`;
assertAnnotated({
ruleName,
message: Rule.FAILURE_STRING,
source
});
});

it('should fail when a relative URL is prefixed by ../', () => {
const source = `
@Component({
templateUrl: '../foobar.html'
wKoza marked this conversation as resolved.
Show resolved Hide resolved
~~~~~~~~~~~~~~~~
})
class Test {}
`;
assertAnnotated({
ruleName,
message: Rule.FAILURE_STRING,
source
});
});

it('should fail when a relative URL is prefixed by ../', () => {
const source = `
@Component({
templateUrl: '.././foobar.html'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excuse me for the misleading comment last time, to me it makes sense to throw a warning on ./../ and succeed on ../. The prefix in the first case seem redundant.

Copy link
Collaborator Author

@wKoza wKoza Nov 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The style guide Angular says:

Why? A component relative URL requires no change when you move the component files, as long as the files stay together.

I understood that like 'in the same folder' (mainly in an Angular CLI environment) but, indeed, it can lead to different interpretations.

Our rationale is when you move the component files. So, we should accept all relative paths.
In my IDE, if I move a component with a relative url, my IDE fixes the new path automatically.
I agree that ./../ is unclean but, for me, it's the same thing than ../.

Do you know what I mean ? I can fix it quickly with this in mind (we accept all relative paths).

~~~~~~~~~~~~~~~~~~
})
class Test {}
`;
assertAnnotated({
ruleName,
message: Rule.FAILURE_STRING,
source
});
});
});
});
});