-
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
Conversation
c001971
to
209bb80
Compare
209bb80
to
ecb8cd5
Compare
super(sourceFile, options); | ||
} | ||
|
||
visitClassDecorator(decorator: ts.Decorator) { |
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.
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.
I left some comments/questions. The rule makes sense and we can go ahead and merge it when we figure out the open questions.
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 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.
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.
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).
The ./ prefix is standard syntax for relative URLs; don't depend on Angular's current ability to do without that prefix.
Rationale: A component relative URL requires no change when you move the component files, as long as the files stay together.
related to #623