search

patrickhulce / fontmin-webpack

Minifies icon fonts to just the used glyphs.
MIT License
139 stars 19 forks source link

UNICODE_REGEX only finds 4 or 6-bytes glyphs #54

Closed ArTiSTiX closed 2 years ago

ArTiSTiX commented 2 years ago

👋 I am currently testing and implementing fontmin-webpack to reduce the size of Font Awesome 6.0.0.

It works well and almost as expected, but since some Font Awesome icons use existing unicode glyphs if there is semantic-equivalents, some of those glyphs use 2-bytes codes that are not matched by this regex, only 4 and 6 bytes codes: https://github.com/patrickhulce/fontmin-webpack/blob/fcfcad4fcab486c385cf0a314ca6566efddedb54/lib/index.js#L14

This is an issue for all the latin letters and numbers, but also some common icons like "plus", "dollar-sign", etc...

Example of 2-bytes chars from Font Awesome: https://github.com/FortAwesome/Font-Awesome/blob/6.x/scss/_variables.scss#L1208

Could you add 2-bytes unicodes to allow those glyphs ? Or would you expect some unintended behaviour ?

Thanks

NOTE: I was able to fix the issue on my side, temporarily, by overriding chars with a 4-byte form, but i consider it legacy solution.

patrickhulce commented 2 years ago

Thanks for filing @ArTiSTiX! I would expect an increased false positive rate and would be hesitant to turn it on by default, at least at first. Perhaps a middle ground could be making the autodetect option accept a function and document how to handle font awesome prominently in the README?

ArTiSTiX commented 2 years ago

@patrickhulce I don't know if it's common usage to use 2-bytes (and even unicode) in content properties, since it's mostly readable chars below 255, i tend to think developers would not use the hex format, and rather the character directly.

Even though, if we have false positives and the glyph is not present in the font, would it have any effect ?

If you prefer not risk it, a way to tweak the regex is acceptable - note that this is a usecase for fontawesome, its not an uncommon usecase ;)

patrickhulce commented 2 years ago

Yep all fair. Let's do it, just bump the major version 👍

ArTiSTiX commented 2 years ago

@patrickhulce i opened 3 PRs for 3 issues i reported.

Thanks for your feedbacks, and hope we will be able to have the 3 fixes in a major bumped version. I noticed the package version is still at 0.0.0, i suppose you bump this when publishing.