New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trim down the text emoji regex size #15

Closed
wants to merge 1 commit into
base: master
from

Conversation

2 participants
@gilmoreorless
Contributor

gilmoreorless commented Apr 5, 2017

I noticed the text emoji regex was using an inefficient method of optionally matching the variation selector \uFE0F after \p{Emoji}. Due to all the character class expansions, this ends up bloating the generated regex with duplicate paths. Switching the match to just make \uFE0F optional brings down the size of the generated regex considerably:

> require('./text-old')().source.length
8474
> require('./text-new')().source.length
7093

I also added a looped test for all the \p{Emoji} characters to verify it still matches everything correctly.

@mathiasbynens

LGTM with that minor nit addressed

for (const codePoint of Emoji) {
test(String.fromCodePoint(codePoint));
}

This comment has been minimized.

This comment has been minimized.

@gilmoreorless

gilmoreorless Apr 6, 2017

Contributor

When I do that it produces 207 failures, as line 41 is in the section testing the index regex instead of the text one. It becomes a case of testing the Emoji symbols by themselves against the index regex that expects them to be always be followed by \uFE0F, hence the failures.

This comment has been minimized.

@mathiasbynens

mathiasbynens Apr 6, 2017

Owner

You’re completely right — my bad! I’ll merge as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment