feat!: support parseable URL values#158
Conversation
|
@CharlieHelps use the pull request template |
|
Done — I updated the PR body to use the repository pull request template and kept the |
shellscape
left a comment
There was a problem hiding this comment.
I see no tests for the myriad of url('...') variants. And since url is producing a Word, the tests should go into the word tests.
|
@gwynne I was able to get a review in before I had to step away. please review this when able |
gwynne
left a comment
There was a problem hiding this comment.
Other than the incomplete test coverage, the rest looks correct to me, and a quick local test shows that it fully covers my use case.
| { fixture: 'url(https://example.com/image.png)', label: 'unquoted absolute URL' }, | ||
| { fixture: "url('https://example.com/image.png')", label: 'single-quoted absolute URL' }, | ||
| { fixture: 'url("https://example.com/image.png")', label: 'double-quoted absolute URL' }, | ||
| { fixture: 'url(/images/image.png)', label: 'unquoted relative URL' }, | ||
| { fixture: "url('/images/image.png')", label: 'single-quoted relative URL' }, | ||
| { fixture: 'url("/images/image.png")', label: 'double-quoted relative URL' }, | ||
| { fixture: 'url(//cdn.example.com/image.png)', label: 'protocol-relative URL' }, | ||
| { fixture: 'url()', label: 'empty URL value' } |
There was a problem hiding this comment.
These are both mislabeled and incomplete. There are three types of meaningful URL—protocol-absolute, file-absolute, and file-relative—and valid inputs can include both query strings (e.g. ?a=b) and fragment identifiers (e.g. #foo). Procotol-relative URLs, while they did historically exist, are not considered valid by modern browsers, and thus should not be considered IMO. Here is a more comprehensive test matrix:
| { fixture: 'url(https://example.com/image.png)', label: 'unquoted absolute URL' }, | |
| { fixture: "url('https://example.com/image.png')", label: 'single-quoted absolute URL' }, | |
| { fixture: 'url("https://example.com/image.png")', label: 'double-quoted absolute URL' }, | |
| { fixture: 'url(/images/image.png)', label: 'unquoted relative URL' }, | |
| { fixture: "url('/images/image.png')", label: 'single-quoted relative URL' }, | |
| { fixture: 'url("/images/image.png")', label: 'double-quoted relative URL' }, | |
| { fixture: 'url(//cdn.example.com/image.png)', label: 'protocol-relative URL' }, | |
| { fixture: 'url()', label: 'empty URL value' } | |
| { fixture: 'url(https://example.com/image.png)', label: 'unquoted full URL' }, | |
| { fixture: 'url(/images/image.png)', label: 'unquoted absolute file path' }, | |
| { fixture: 'url(images/image.png)', label: 'unquoted relative file path' }, | |
| { fixture: 'url(./images/image.png)', label: 'unquoted relative file path with leading ./' }, | |
| { fixture: "url('https://example.com/image.png')", label: 'single-quoted full URL' }, | |
| { fixture: "url('/images/image.png')", label: 'single-quoted absolute file path' }, | |
| { fixture: "url('images/image.png')", label: 'single-quoted relative file path' }, | |
| { fixture: "url('./images/image.png')", label: 'single-quoted relative file path with leading ./' }, | |
| { fixture: 'url("https://example.com/image.png")', label: 'double-quoted full URL' }, | |
| { fixture: 'url("/images/image.png")', label: 'double-quoted absolute file path' }, | |
| { fixture: 'url("images/image.png")', label: 'double-quoted relative file path' }, | |
| { fixture: 'url("./images/image.png")', label: 'double-quoted relative file path with leading ./' }, | |
| { fixture: "url('https://example.com/image.png?1234567890#abcdef')", label: 'single-quoted full URL with query and fragment' }, | |
| { fixture: "url('/images/image.png?1234567890#abcdef')", label: 'single-quoted absolute file path with query and fragment' }, | |
| { fixture: "url('images/image.png?1234567890#abcdef')", label: 'single-quoted relative file path with query and fragment' }, | |
| { fixture: "url('./images/image.png?1234567890#abcdef')", label: 'single-quoted relative file path with leading ./, query, and fragment' }, | |
| { fixture: 'url("https://example.com/image.png?1234567890#abcdef")', label: 'double-quoted full URL with query and fragment' }, | |
| { fixture: 'url("/images/image.png?1234567890#abcdef")', label: 'double-quoted absolute file path with query and fragment' }, | |
| { fixture: 'url("images/image.png?1234567890#abcdef")', label: 'double-quoted relative file path with query and fragment' }, | |
| { fixture: 'url("./images/image.png?1234567890#abcdef")', label: 'double-quoted relative file path with leading ./, query, and fragment' }, | |
| { fixture: 'url()', label: 'empty URL value' }, | |
| { fixture: "url('')", label: 'empty single-quoted URL value' }, | |
| { fixture: 'url("")', label: 'empty double-quoted URL value' } |
This PR contains:
Breaking Changes?
If yes, please describe the breakage.
The value of
isUrlis now determined by different conditions, which could break consumers downstream if relying on older behavior and value ofisUrl.Please Describe Your Changes
Resolves #157