Skip to content

fix(diacritics): compose Hangul back after stripping, so Korean search keeps working - #861

Open
wonjangcloud9 wants to merge 1 commit into
krisk:mainfrom
wonjangcloud9:fix/ignore-diacritics-hangul
Open

wonjangcloud9 wants to merge 1 commit into
krisk:mainfrom
wonjangcloud9:fix/ignore-diacritics-hangul

Conversation

@wonjangcloud9

Copy link
Copy Markdown

Problem

With ignoreDiacritics: true, Korean search matches the wrong items and highlights the wrong text.

stripDiacritics NFD-normalizes its input to split off combining marks. NFD also decomposes every Hangul syllable into two or three conjoining jamo, which aren't diacritics and so are never stripped, and nothing recomposes them. The pattern and the text are compared as jamo sequences, so syllables that share a consonant fuzzily match each other. Match indices also come back as offsets into the longer decomposed string.

const list = ['감자', '검정', '곰', '김치', '강아지']
new Fuse(list, { threshold: 0.4 }).search('감')                          // ['감자']
new Fuse(list, { threshold: 0.4, ignoreDiacritics: true }).search('감')  // ['감자', '검정', '곰', '김치', '강아지']

new Fuse([{ t: '한국어 검색 테스트' }], { keys: ['t'], includeMatches: true, ignoreDiacritics: true })
  .search('검색')[0].matches[0].indices                                   // [[9, 14]] -> '트'

Fix

stripDiacritics now ends with .normalize('NFC'). That recomposes whatever NFD split but no rule removed, which means Hangul. Combining marks are already gone by then, so Latin results don't change (é still matches e). Precomposed Hangul keeps its original length, so its match indices line up with the original text again.

Results with the built dist/fuse.mjs, main vs this branch:

main this branch
search('감'), ignoreDiacritics: true ["감자","검정","곰","김치","강아지"] ["감자"] (same as without the option)
highlight for 검색 in 한국어 검색 테스트 [[9,14]] → 트 [[4,5]] → 검색
Crème brûlée 레시피 found by creme brulee 레시피, threshold: 0 yes yes

Tests

New cases in test/fuzzy-search.test.js:

  • Searching Korean text ignoring diacritics checks that a syllable gives the same results as without the option, and that match indices point at the matched syllables. Both fail on main and pass here.
  • A Latin + Hangul guard checks that accents are still stripped. It passes on both.

npm run build, npm test (22 files, 452 tests) and npm run lint pass. As in other fixes, dist/ is rebuilt from the source change; each file differs only in the stripDiacritics line.

Performance

I measured search time over 10,000 items with ignoreDiacritics: true, alternating main's and this branch's dist over 15 trials each:

  • Latin items: same within noise (14.7 vs 15.2 ms in one run, 32.6 vs 29.3 ms in another).
  • Korean items: about 3× faster (28.9 → 9.2 ms, 57.2 → 19.7 ms), since the strings being compared are shorter again.

Related

#840 remaps match indices for transformations that change string length, and touches this file. It doesn't change the Korean matching itself. This PR is a one-line change in stripDiacritics, so whichever lands second should rebase easily.

🤖 Generated with Claude Code

…h keeps working

stripDiacritics NFD-normalizes its input to split off combining marks, but
NFD also splits every Hangul syllable into two or three conjoining jamo,
none of which is a diacritic, and nothing put them back together. With
ignoreDiacritics on, a Korean query matched by shared jamo instead of by
syllable: searching 감 among 감자, 검정, 곰, 김치, 강아지 returned all five
instead of 감자. Match indices pointed into the longer decomposed text, so
the highlight for 검색 in "한국어 검색 테스트" landed on 트.

The stripped string is now NFC-normalized, which recomposes what NFD split
but no rule removed. Latin results are unchanged (é still matches e), and
precomposed Hangul keeps its length, so its match indices line up again.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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.

1 participant