Skip to content
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

🎄🔨 Force tree shake emojione #4202

Merged
merged 8 commits into from
Jul 14, 2017
Merged

🎄🔨 Force tree shake emojione #4202

merged 8 commits into from
Jul 14, 2017

Conversation

sorin-davidoi
Copy link
Contributor

Based on #4189, will rebase as soon as it is merged.

We use babel-plugin-preval to cherry-pick what we need from emojione and avoid shipping the entire library to the browsers. The entire library is still imported by emoji-picker, but it gets included in that bundle, which is only downloaded when the emoji drop-down is expanded.

Effect on the common bundle:

Before After Difference
Parsed 749 KB 493 KB 🔻 -34%
Gziped 182 KB 142 KB 🔻 -22%

Bundle view - notice how small emojione_light is compared to emojione.

cc: @nolanlawson

@sorin-davidoi sorin-davidoi changed the title 🎄 Force tree shake emojione 🔨🎄 Force tree shake emojione Jul 14, 2017
@sorin-davidoi sorin-davidoi changed the title 🔨🎄 Force tree shake emojione 🎄🔨 Force tree shake emojione Jul 14, 2017
@Gargron
Copy link
Member

Gargron commented Jul 14, 2017

Now that the original PR is merged, can you try rebasing on top of master?

@sorin-davidoi
Copy link
Contributor Author

Rebased. Also added a preload script for emojione_picker, since it is bigger now.

@Gargron Gargron merged commit c1f201c into mastodon:master Jul 14, 2017
@sorin-davidoi sorin-davidoi deleted the force-tree-shake-emojione branch July 14, 2017 18:30
@sorin-davidoi
Copy link
Contributor Author

Thank you @kentcdodds for this library, saves us quite a few bytes!

@unarist
Copy link
Contributor

unarist commented Jul 14, 2017

Oh, this removed title attribute? I think :emoji: syntax is still useful for manual input, and title attribute helps knowing its shortname.

@sorin-davidoi
Copy link
Contributor Author

Yes, but displaying that (on hover, so only on desktop) means pulling in the entire array, which is big. I would argue that it is not worth it - you can always see the shortcode in the emoji picker.

@kentcdodds
Copy link

So glad that preval helped! Good work here!

@milesj
Copy link

milesj commented Jul 17, 2017

Just chiming in here. Have you look at something like emoji-database over emojione?

@sorin-davidoi
Copy link
Contributor Author

We haven't. The thing is that we already use emojione-picker, which brings in emojione, so we would rather use it since it will be fetched and bundled anyway. There has been some talk about switching to another emoji set, if that happens we will definitely look into it!

@milesj
Copy link

milesj commented Jul 17, 2017

No worries, makes sense! If you guys do use it, and have any feedback, feel free to send it my way!

@nolanlawson
Copy link
Contributor

This is awesome. Another cool thing about this PR is that it will take less time to evaluate the JS, because if you look at emojione's source you'll see it does a lot of work to build up regexes and object maps.

It's a bit of a shame we no longer have the title, but potentially a low cost way to bring it back would be to pre-eval a trie structure. Or as @sorin-davidoi says, we can just accept that it's gone because perhaps it's not really worth it.

@Gargron
Copy link
Member

Gargron commented Jul 18, 2017

Just FYI people have since expressed interest in the title attribute, I think there is a high demand for it. It doesn't only teach people how to invoke the emojis without access to a unicode keyboard/utility, but it helps recognize emoji that are hard to understand visually.

@kaniini
Copy link
Contributor

kaniini commented Jul 18, 2017

the only thing i have interest in is reverting this merge. it makes the style of the emojis inconsistent, and worse, breaks terribly on linux if you do not have a font that correctly supports emojis.

@sorin-davidoi
Copy link
Contributor Author

This PR does not affect how emojis are rendered. The only visual change it brings is that it removes the title attribute, which we will try to bring back due to the feedback received.

@nightpool nightpool mentioned this pull request Aug 7, 2017
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants