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

refactor(formatters): Change :_: emoji name placeholder #10567

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

cobaltt7
Copy link
Contributor

Please describe the changes this PR makes and why it should be merged:

Discord doesn't verify emoji names, meaning you only actually need to include a valid ID for an emoji to display properly. Discord.js uses _ as a fallback name when formatting emojis with just an ID. However, since _ is not a valid emoji name, if you were to parse that emoji, it would fail, which could be unexpected behavior.
image
image

Additionally, if you were to have multiple emojis with the _ name placeholder and invalid IDs (or if the bot doesn't have access to the emoji), they fail to render in Discord and the text between them becomes unintentionally italicized. For example, <:_:123678901234578> foobar <:_:123678901234578>
image
This is another unintentional side-effect of using :_: as a placeholder emoji name that end-users currently have to look out for.

To fix this, I changed the :_: placeholder to :emoji:. This way it is a valid emoji name that parses correctly and doesn't interfere with any other Markdown features. Originally, I was going to change it to :__:, which would parse correctly, but causes text between invalid emojis to be underlined.

In my opinion, parseEmoji should also be changed to properly support emojis that used :_: in the past, because even though it's not a valid emoji name, it does still render correctly, but people in the support server disagreed
image

Status and versioning classification:

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

@cobaltt7 cobaltt7 requested a review from a team as a code owner October 20, 2024 19:28
Copy link

vercel bot commented Oct 20, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments
Name Status Preview Comments Updated (UTC)
discord-js ⬜️ Ignored (Inspect) Visit Preview Nov 7, 2024 4:39am
discord-js-guide ⬜️ Ignored (Inspect) Visit Preview Nov 7, 2024 4:39am

Copy link

codecov bot commented Oct 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 38.02%. Comparing base (ed78e45) to head (4e90578).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10567      +/-   ##
==========================================
+ Coverage   38.01%   38.02%   +0.01%     
==========================================
  Files         239      239              
  Lines       15471    15466       -5     
  Branches     1353     1353              
==========================================
  Hits         5881     5881              
+ Misses       9575     9570       -5     
  Partials       15       15              
Flag Coverage Δ
formatters 97.47% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@Jiralite Jiralite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally would rather keep this as-is for the current minor version and make name required in the future. Thoughts? @vladfrangu @almeidx

@vladfrangu
Copy link
Member

I personally would rather keep this as-is for the current minor version and make name required in the future. Thoughts? @vladfrangu @almeidx

🤷. We can also just have a fallback on unknown_emoji, and recommend users pass in the emoji name (but theres plenty times where you only have an ID but can still react). I personally don't think we need to make it required

@Jiralite Jiralite added this to the formatters 0.6.0 milestone Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

4 participants