Updates no-url-protocols rule and adds additional no-domains rule #846
Conversation
danwaz
changed the title
|
Hey @danwaz would you mind updating the branch again I just had to finish merging the release branch in. I can review it for you after that. Thanks for the PR! 👍 |
|
@DanPurdy No problem! Updating now. |
|
|
||
| module.exports = { | ||
| 'name': 'no-url-protocols', | ||
| 'defaults': {}, | ||
| 'defaults': { | ||
| 'protocol-relative-urls': false |
There was a problem hiding this comment.
I think this needs a positive or negative prefix, such as allow-protocol-relative(-urls)? This would reverse the logic slightly and mean you could remove the logical operators when assigning regexSelector and message below.
Overall i feel it's more declarative and less ambiguous about its intentions when a user comes across it.
There was a problem hiding this comment.
I added allow-protocol-relative-urls here. I agree, much less ambiguous.
|
Hey @danwaz, I've left a few comments for discussion, looking forward to hearing your thoughts. Appreciate the contribution 👍 |
|
Awesome, looking good! I'll give it a once over again in the morning (it's late here!) and merge it in if all is good. You may want to add your 'Huge' email address as a secondary to your github account too for proper attribution of those commits! 😉 Resolves #813 |
|
Thanks @DanPurdy! H/t for the email suggestion. |
| @@ -25,6 +25,7 @@ rules: | |||
| no-css-comments: 1 | |||
| no-debug: 1 | |||
| no-disallowed-properties: 0 | |||
| no-url-domains: 1 | |||
There was a problem hiding this comment.
Could you add this in alphabetical order within this block please
|
Just one more change and then this is good to go! |
|
🎉 🎉 Thanks @danwaz ! |
| var stripped = helpers.stripQuotes(item.content), | ||
| parsedUrl = url.parse(stripped, false, true); | ||
|
|
||
| if (parsedUrl.host && parsedUrl.protocol !== 'data:') { |
There was a problem hiding this comment.
What about //assets.example.com/some-font.ttf ? It fails on this and I think it shouldn't.
Addresses issue #813
This pull request addresses the conflation of the no-url-protocols and no-domains rules. In addition, it also adds a configuration option to allow for protocol-relative-urls (i.e.
//some-domain.com/foo.png) within the no-url-protocols rule.As discussed in issue #813, this new configuration should not add any breaking changes if users decide to update to this version. All additional configurations are set to
falseby default. Also to keep in mind, when the project updates to v2, all legacy no-domain logic should be removed from the no-url-protocols rule.Unit tests and documentation have been updated to reflect the new rule and additional configuration.
<DCO 1.1 Signed-off-by: Dan Wasilewski heydanwaz@gmail.com>