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

fix: cross-platform emoji compatibility #1449

Merged
merged 18 commits into from
Aug 19, 2024
Merged

Conversation

setchy
Copy link
Member

@setchy setchy commented Aug 8, 2024

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

Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy setchy added this to the Release 5.13.0 milestone Aug 8, 2024
@github-actions github-actions bot added bug Something isn't working dependency Dependency updates labels Aug 8, 2024
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy
Copy link
Member Author

setchy commented Aug 8, 2024

@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 🙂

@setchy setchy added the platform:windows Related to windows build label Aug 8, 2024
@Araxeus
Copy link
Contributor

Araxeus commented Aug 8, 2024

How do I easily test the changes? im not sure how to display them in the app? they are locked behind certain conditions right?

@setchy
Copy link
Member Author

setchy commented Aug 8, 2024

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 Network Connectivity error page which you previously had a broken emoji for

@Araxeus
Copy link
Contributor

Araxeus commented Aug 8, 2024

image

Doesn't seem to work on my Windows machine, does it actually work on your pc?

btw the last time i got this page wasn't because i lost connection but rather because my pc went to sleep and woke up

Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy
Copy link
Member Author

setchy commented Aug 8, 2024

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.

src/components/icons/Emoji.tsx Outdated Show resolved Hide resolved
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@Araxeus
Copy link
Contributor

Araxeus commented Aug 9, 2024

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 🤷‍♂️

I have trauma from polyfill.io cdn drama

ps. it works now

image

@setchy
Copy link
Member Author

setchy commented Aug 9, 2024

Thanks for retesting @Araxeus and confirming it works now.

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

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.

Those svg files could even be included in an npm dependency if needed

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)

@Araxeus
Copy link
Contributor

Araxeus commented Aug 9, 2024

I did look around for a while but couldn't find any maintained packages for twemoji svgs (eg: 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 postinstall (or a versioned https://github.com/jdecked/twemoji/tree/v15.1.0/assets if we want to be safer)


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)

@setchy
Copy link
Member Author

setchy commented Aug 9, 2024

Left a comment on jdecked/twemoji#58 - let's see 🙏

Looks like the discord fork https://github.com/discord/twemoji does package SVGs under /dist, so perhaps that's an interim route... Does look like jdecked wants to help retire that fork in time.

Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy
Copy link
Member Author

setchy commented Aug 9, 2024

Pivoted to https://github.com/discord/twemoji to source all emojis locally from node_modules. Thanks for the great feedback @Araxeus 🙇

Signed-off-by: Adam Setch <adam.setch@outlook.com>
@Araxeus
Copy link
Contributor

Araxeus commented Aug 9, 2024

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 <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@afonsojramos
Copy link
Member

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

Agree with the above!

@setchy setchy changed the title fix: cross-platform emoji compatibility 🐛 fix: cross-platform emoji compatibility Aug 12, 2024
@github-actions github-actions bot removed the bug Something isn't working label Aug 12, 2024
@setchy setchy changed the title 🐛 fix: cross-platform emoji compatibility fix: cross-platform emoji compatibility Aug 12, 2024
@github-actions github-actions bot added the bug Something isn't working label Aug 12, 2024
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
Signed-off-by: Adam Setch <adam.setch@outlook.com>
@setchy
Copy link
Member Author

setchy commented Aug 14, 2024

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.

@Araxeus
Copy link
Contributor

Araxeus commented Aug 14, 2024

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

@bmulholland
Copy link
Collaborator

@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.

@Araxeus
Copy link
Contributor

Araxeus commented Aug 15, 2024

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 💪

@setchy
Copy link
Member Author

setchy commented Aug 15, 2024

@Araxeus - certainly not the "bad guy" at all - I sincerely appreciate the engagement, feedback and discussion. Huge thank you 🙏

Copy link
Member

@afonsojramos afonsojramos left a 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! 🚀

Copy link
Member

@afonsojramos afonsojramos left a 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! 🚀

@afonsojramos afonsojramos merged commit 88ba960 into main Aug 19, 2024
12 checks passed
@afonsojramos afonsojramos deleted the fix/emoji-compatability branch August 19, 2024 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependency Dependency updates platform:windows Related to windows build
Projects
None yet
Development

Successfully merging this pull request may close these issues.

review use of emojis for cross-platform compatability
4 participants