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

Conversation

wKoza
Copy link
Collaborator

@wKoza wKoza commented Nov 23, 2018

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

@wKoza wKoza force-pushed the relativeUrlPrefixRule branch from c001971 to 209bb80 Compare November 23, 2018 20:25
@wKoza wKoza force-pushed the relativeUrlPrefixRule branch from 209bb80 to ecb8cd5 Compare November 23, 2018 20:27
@wKoza wKoza requested review from mgechev and rafaelss95 November 23, 2018 20:34
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.

Copy link
Owner

@mgechev mgechev left a 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'
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).

@mgechev mgechev merged commit f12f27b into mgechev:master Nov 25, 2018
@wKoza wKoza deleted the relativeUrlPrefixRule branch November 25, 2018 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants