tigt / mini-svg-data-uri

Small, efficient encoding of SVG data URIs for CSS, HTML, etc.
https://npm.runkit.com/mini-svg-data-uri
MIT License
309 stars 16 forks source link

Shorter color names don't check the entire hexcode #3

Closed cascornelissen closed 6 years ago

cascornelissen commented 6 years ago

Input

const miniSvgDataUri = require("mini-svg-data-uri");
miniSvgDataUri('#f00999');

Expected output

"data:image/svg+xml,%23f00999"

Actual output

"data:image/svg+xml,red999"
cascornelissen commented 6 years ago

A possible solution for this issue would be to improve the regexes in shorter-css-color-names.js to make sure they check that whatever is behind the match is not a valid alphanumerical character.

The regex for red would change from:

/#ff0000|#f00/gi

to:

/#ff0000|#f00(?!\w)/gi

This solution uses a negated lookahead (!?...) and the meta sequence for any word character \w and would work in the following examples:

#f00            # Match
#ff0000         # Match
fill="#f00"     # Match
fill="#ff0000"  # Match
#f00999         # No match
abc#f00999cba   # No match
#f00f00         # No match
fill="#f00f00"  # No match

@tigt, since it's been 4 months since the last commit on this repository, does this look like a good solution to you and would you accept a PR?

tigt commented 6 years ago

Yep, this is still active, it's just small and simple so it doesn't have much going on.

I think we might be able to get away with just the trailing quote; the hexes can show up in other contexts with CSS, but minification should convert those to attributes where possible.

tigt commented 6 years ago

This honestly surprises me, because I thought the regex engine would try to match the first sequence, and only then the second, shorter one. Learn something every day, I guess.

cascornelissen commented 6 years ago

You mean changing the regex to include the trailing quote? Any reason to pick that over the negated lookahead as that would work with other contexts as well (matches all 3 occurrences):

color: #f00;
background: linear-gradient(to left, #f00, #ff0000);
tigt commented 6 years ago

Yeah, that's probably better.

cascornelissen commented 6 years ago

Alright, let me know if you'd like me to submit a PR.

Thanks for answering this quick, by the way!

tigt commented 6 years ago

I'd be happy if you did; you seem to understand the problem better than I.

cascornelissen commented 6 years ago

@tigt, could you maybe publish this to npm as well?

tigt commented 6 years ago

Oh, whoops, certainly. Thank you for reminding me — can you tell this is my first open-source thingo?

tigt commented 6 years ago

Should be republished; let me know if I did it wrong.

cascornelissen commented 6 years ago

Awesome, looks like 1.0.1 is available on npm and it contains the fixes 👍

Thanks! 😄