fix: strip and replace emoji that have a skin tone modifier - #321
Open
spokodev wants to merge 1 commit into
Open
fix: strip and replace emoji that have a skin tone modifier#321spokodev wants to merge 1 commit into
spokodev wants to merge 1 commit into
Conversation
`strip()` and `replace()` left skin-tone-modified emoji in place, even
though `which()` and `unemojify()` recognise them:
strip('X馃憤馃徑Y') // 'X馃憤馃徑Y' (expected 'XY')
replace('馃憤馃徑', () => 'Z') // '馃憤馃徑' (expected 'Z')
which('馃憤馃徑') // '+1' (recognised)
`replace()` looked the character up with `findByCode(character)` on the raw
character, while `which()` first normalises the skin tone with
`findByCode(skinTone(emoji, 'none'))`. Apply the same normalisation in
`replace()` (which `strip()` delegates to). `skin-tone` is already a
dependency, and the base table contains no skin-tone scalars, so plain
emoji are unaffected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
strip()andreplace()leave skin-tone-modified emoji in place, even thoughwhich()andunemojify()recognise them:char-regexalready tokenises馃憤馃徑as one character, so the gap is purely in the lookup. The README documentsstripas "Remove all of the emojis from a string" andreplaceas "Replace the emojis in a string", and a skin-toned emoji is unambiguously an emoji (the lib's ownwhichreturns+1for it), so leaving it in place is a contract/consistency violation.Cause
replace()looks the character up withfindByCode(character)on the raw character, whilewhich()first normalises the skin tone:strip()delegates toreplace(), so both are affected.Fix
Apply the same skin-tone normalisation in
replace()thatwhich()already uses.skin-toneis already a dependency, and the base shortcode table contains no skin-tone scalars, so plain emoji and existing matches are unaffected.Tests
Added skin-tone cases to
strip.test.tsandreplace.test.ts. They fail onmainand pass with the fix. Full suite green (74 tests), including the existing complex-ZWJ, flag, andpreserveSpacescases.