Fix: Module types should support urls without quotes #2467
Conversation
| .then(function (source) { | ||
| source = source.replace(/url\(\s*(?:(["'])((?:\\.|[^\n\\"'])+)\1|((?:\\.|[^\s,"'()\\])+))\s*\)/g, function (match, quotes, relUrl1, relUrl2) { | ||
| return 'url(' + quotes + resolveUrl(relUrl1 || relUrl2, url) + quotes + ')'; | ||
| return ['url(', quotes, resolveUrl(relUrl1 || relUrl2, url), quotes, ')'].join(''); |
There was a problem hiding this comment.
Can you please explain the bug that this is resolving? This refactoring looks entirely equivalent to me?
There was a problem hiding this comment.
Sure, I should have probably added the following to the PR description...
It's valid CSS to not include quotes in urls like background-image: url(my/image.png);. However the code above that tries to resolve urls doesn't seem to cater for this. So if quotes is undefined the string concatenation adds undefined to the result and the image fails to load. e.g.
'url(' + undefined + 'my/image.png' + undefined + ')' // --> "url(undefinedmy/image.pngundefined)"However using array.join we can take advantage of the spec to discard unwanted undefined from the result.
If element0 is undefined or null, let R be the empty String; otherwise, Let R be ToString(element0).
e.g.
['url(', undefined, 'my/image.png', undefined, ')'].join('') // --> "url(my/image.png)" I'm more than happy to refactor if you feel the approach I've taken isn't explicit enough.
|
Thanks for clarifying, makes sense now! |
|
@guybedford latest version 6.14.2 still not working. maybe need release new one. system.js is correct |
This PR addresses the issue of undefined being concatenated into url strings when urls aren't wrapped in quotes.
I believe the fixture css I've added is valid for urls. I've not used chomp before. It changed some dist files when I ran it locally but I'm guessing you wouldn't want me to commit these changes in the PR. 🤔
fixes: #2460