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

feat: support consideration for surrogate pairs #4229

Merged
merged 5 commits into from
Apr 30, 2023
Merged

Conversation

NaokiHaba
Copy link
Contributor

@NaokiHaba NaokiHaba commented Apr 28, 2023

🔎 Overview

🤓 Code snippets/examples (if applicable)

It appears that the current implementation of the String.length method may not accurately reflect the number of characters in a string containing surrogate pairs.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/length

Specifically, this is the case
https://codepen.io/nao62165/pen/OJBgwjB?editors=1111

With this change, users of surrogate pairs will also receive the expected results.

We hope you will consider this!

The method of using Intl.Segmenter is also mentioned. Segmenter](), but it is not available in FireFox at this time. Spread syntax has been adopted so that more users can use it.

// some code
[...String(value)].length <= Number(length);

Issues affected

@NaokiHaba NaokiHaba marked this pull request as ready for review April 28, 2023 05:59
Copy link
Owner

@logaretm logaretm left a comment

Choose a reason for hiding this comment

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

Good one, thank you! Can you add tests that verifies this? Happy to merge if you don't have the time.

@NaokiHaba
Copy link
Contributor Author

Good one, thank you! Can you add tests that verifies this? Happy to merge if you don't have the time.

Thank you for confirming!

We have added a test case for this change

Copy link
Owner

@logaretm logaretm left a comment

Choose a reason for hiding this comment

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

Splendid, merging it now and will tag a patch release tomorrow! Thanks for the quick reaction.

@logaretm logaretm merged commit b6dd1fc into logaretm:main Apr 30, 2023
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.

2 participants