-
Notifications
You must be signed in to change notification settings - Fork 235
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) { | ||
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); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The style guide Angular says:
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 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 | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
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.