Match parens recursively on URLs to not fix embeded calls #192
Conversation
sontek
force-pushed
the
fix_embeded_parens_matching_recursively
branch
from
797199d
to
07ea718
Compare
|
@ekulabuhov @michael-ciniawsky @tquetano-r7 Please review |
|
@bebraw @ekulabuhov ping 😛 |
fixUrls.js
Outdated
| @@ -29,8 +29,9 @@ module.exports = function (css) { | |||
| var currentDir = baseUrl + location.pathname.replace(/\/[^\/]*$/, "/"); | |||
|
|
|||
| // convert each url(...) | |||
| var fixedCss = css.replace(/url *\( *(.+?) *\)/g, function(fullMatch, origUrl) { | |||
| var fixedCss = css.replace(/url\(((?:[^)(]+|\((?:[^)(]+|\([^)(]*\))*\))*)\)/g, function(fullMatch, origUrl) { | |||
There was a problem hiding this comment.
Yeah. It would be better for the next coder if you split that up a bit (even in a comment). Scary to read.
There was a problem hiding this comment.
Would it be acceptable to bring in a dependency instead? This regex is only necessary because standard regex in the browser doesn't support recursive matching.
https://www.npmjs.com/package/xregexp could do this in a cleaner manner.
I'll try to explain it in a comment but its basically just a nasty way to recursively match brackets
There was a problem hiding this comment.
It would be fine to me (simpler code is better, also less to maintain this way). Better wait what others think.
There was a problem hiding this comment.
That lib itself has no dependencies, is small and looks to be well maintained. Normally I'd say just use the regex but he has a point about browser regex being a bit gimp.
There was a problem hiding this comment.
Ok, let's go with a dep then. 👍
There was a problem hiding this comment.
I added a comment on there that might clear it up so we don't need the dep, what do you think?
|
|
||
| /gi = Get all matches, not the first. Be case insensitive. | ||
| */ | ||
| var fixedCss = css.replace(/url\s*\(((?:[^)(]|\((?:[^)(]+|\([^)(]*\))*\))*)\)/gi, function(fullMatch, origUrl) { |
There was a problem hiding this comment.
Well, this got complicated pretty fast 😄
Another options to visualise it: https://regexper.com/#%2Furl%5Cs*%5C(((%3F%3A%5B%5E)(%5D%7C%5C((%3F%3A%5B%5E)(%5D%2B%7C%5C(%5B%5E)(%5D*%5C))*%5C))*)%5C)%2Fgi
There was a problem hiding this comment.
That is a pretty awesome site!
There was a problem hiding this comment.
It's how I survive dealing with any regex beyond the basics, second bookmark on my chrome bar.
Also super useful - https://regex101.com/
There was a problem hiding this comment.
🤣 http://regexr.com / http://regexr.com/3fhgn here. Glad not to be the only one
|
Great work 👍 I think it's good to go. |
|
Noticed previous comments: |
|
#193 is to visualize how it'd look with |
|
The code is simpler but in general I don't mind this PR, and it always sucks to add an extra dep |
|
I vote #192 to avoid the dep, we have this ticket + the comment in the PR to document the regex. Once you squint at it a bit its not that bad :) |
Fixes #191 / adds test