-
Notifications
You must be signed in to change notification settings - Fork 259
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
fix: cross-platform emoji compatibility #1449
Conversation
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
@Araxeus - if you happy to have a few minutes, I'd really appreciate to test this branch locally and confirm that this fixes the emoji compatibility issues on Windows 10. If not, that's OK too 🙂 |
How do I easily test the changes? im not sure how to display them in the app? they are locked behind certain conditions right? |
Turning off your devices Internet connection (WiFi, etc) should trigger the |
Signed-off-by: Adam Setch <[email protected]>
Apologies for that @Araxeus - I hadn't thought through that when offline, the app wouldn't be able to fetch the SVG from the CDN 🤦. I've pushed an update which adds conditional offline/online handling to our emoji -> twemoji svgs parsing. |
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
I've only just realized they are fetched from cdn... personally I'm not a big fan of this approach - emojis are simple immutable things which could all just be stored locally Those svg files could even be included in an npm dependency if needed But i might be a minority with this opinion 🤷♂️
|
Thanks for retesting @Araxeus and confirming it works now.
I agree, this was my initial reaction too. But after some more reading it seems this is a popular approach for x-platform compatibility and used in platforms like Twitter/X (naturally), Discord etc. Once fetched the first time the assets will be cached in the browser. I did consider storing all of the emojis we use as local SVG assets, but was uneasy with the thought of having to maintain them if/as the Unicode Emoji spec evolves.
I did look around for a while but couldn't find any maintained packages for twemoji svgs (eg: https://www.npmjs.com/package/@twemoji/svg) |
should be pretty simple to remake this library, i dont know why the author turned off auto updates but it should be kept in sync via github actions - https://github.com/boywithkeyboard-archive/twemoji_svg/blob/b5b7226b48d30c215d45f98cbc55691b616ab685/.github/workflows/update.yml https://github.com/boywithkeyboard-archive/twemoji_svg/blob/main/update.ts https://github.com/jdecked/twemoji/tree/gh-pages / https://github.com/jdecked/twemoji/tree/main/assets/svg could also just get the required svg's from https://github.com/jdecked/twemoji/tree/gh-pages on or we could use something like vendorfiles + automated github action to keep the files updated (honestly if this was my repo, its what I would do) |
Left a comment on jdecked/twemoji#58 - let's see 🙏 Looks like the discord fork https://github.com/discord/twemoji does package SVGs under |
Signed-off-by: Adam Setch <[email protected]>
Pivoted to https://github.com/discord/twemoji to source all emojis locally from |
Signed-off-by: Adam Setch <[email protected]>
Is it not better to individually select emoji's to enable tree shaking? (so that we bundle only the required emojis) not super important unless all the svgs weight alot |
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Agree with the above! |
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
Signed-off-by: Adam Setch <[email protected]>
the increased app size with this PR is between 1-2 MB depending on platform. to me, the proposed solution here is adequate enough for what we're trying to solve. |
I just don't think that packaging the whole svg library serves anything except convenience for the devs the end users of the app doesn't actually need all those files, for them it's just bloat (it's not alot but could be avoided) tbh this is really more of a philosophical issue, when you consider the app is already 317MB! of electron bloat on my pc - it doesn't add alot |
@Araxeus If it's indeed 317MB, and the icons are 2MB, then this is 0.6%. From what I understand, e.g. the exe goes from 83->84MB, which is 1.2%. If app size was a concern, which TBH for me it's not, then there's almost certainly bigger gains to be made elsewhere. If you want to talk about overall app size, "electron bloat," etc, can I suggest a discussion focused on that, perhaps as a dedicated issue? We'd first need to align on prioritization between developer experience and app size. And the impact of the app size and memory usage vs that tradeoff. |
You could also add 1000 lines of text to the app and only use one of those lines The app size would go up by less than 1mb 🤷♂️ Honestly i feel like the "bad guy" arguing about this, I wrote like 20 comments on this PR and im not even part of @gitify-app and this isnt important at all - whatever you end up deciding will be fine by me (I wont argue about this anymore) Thank you everyone for the hard work on the app, especially @setchy for the work on this PR 💪 |
@Araxeus - certainly not the "bad guy" at all - I sincerely appreciate the engagement, feedback and discussion. Huge thank you 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ship it! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's ship it! 🚀
Resolves #1446
Use the @discordapp/twemoji (a fork of twemoji which supports locally packaged SVGs) to convert emojis into SVGs for cross-platform compatibility.
twemoji supports the latest Unicode Emoji 15.1