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 implementation to avoid Object.prototype mutation, + perf improvement #47

Merged
merged 3 commits into from
Jul 10, 2017

Conversation

timruffles
Copy link
Contributor

@timruffles timruffles commented Jul 7, 2017

  • use a simple function instead
  • also bumped the minor version as the public API expanded

- also bumped the version a minor, as new functionality is added
@timruffles timruffles changed the title Remove Object.pt mutation Fix implementation to avoid Object.prototype mutation Jul 7, 2017
@timruffles
Copy link
Contributor Author

@omnidan also removed an unnecessary nested loop, as the getKeyByValue thing is more efficiently implemented by inverting the Emoji map.

@timruffles timruffles changed the title Fix implementation to avoid Object.prototype mutation Fix implementation to avoid Object.prototype mutation, + perf improvement Jul 7, 2017
@smeijer
Copy link

smeijer commented Jul 7, 2017

Awesome; I just started a PR, but I'll cancel that one 😁

*/
Emoji.unemojify = function unemojify(str) {
if (!str) return '';
var words = toArray(str);

Choose a reason for hiding this comment

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

var words = Array.from(str);

//cc @paulmillr

Copy link

@bbrzoska bbrzoska Jul 7, 2017

Choose a reason for hiding this comment

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

Indeed, this works properly with Emojis:

> const emoji = '😍🌸'
undefined

> require('lodash').toArray(emoji) // lodash version, understands unicode
[ '😍', '🌸' ]

> [].slice.call(emoji) // slice version, doesn't get unicode chars
[ '�', '�', '�', '�' ]

> Array.from(emoji) // yeey! no need for the dependency, understands unicode
[ '😍', '🌸' ]

Copy link
Contributor

Choose a reason for hiding this comment

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

Does not work for complex emojis. Try for something like 👩‍👩‍👧‍👧. Thats why I used the lodash library.

Copy link

Choose a reason for hiding this comment

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

Aah 😢. Lodash maintainers did a good job with their implementation then!

Copy link

@bbrzoska bbrzoska left a comment

Choose a reason for hiding this comment

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

Better to use the native Array.from, see my inline comment.

*/
Emoji.unemojify = function unemojify(str) {
if (!str) return '';
var words = toArray(str);
Copy link

@bbrzoska bbrzoska Jul 7, 2017

Choose a reason for hiding this comment

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

Indeed, this works properly with Emojis:

> const emoji = '😍🌸'
undefined

> require('lodash').toArray(emoji) // lodash version, understands unicode
[ '😍', '🌸' ]

> [].slice.call(emoji) // slice version, doesn't get unicode chars
[ '�', '�', '�', '�' ]

> Array.from(emoji) // yeey! no need for the dependency, understands unicode
[ '😍', '🌸' ]

@omnidan
Copy link
Owner

omnidan commented Jul 10, 2017

thank you so much for all the effort @timruffles ! this PR looks good and I'll merge it!

@omnidan omnidan merged commit a028147 into omnidan:master Jul 10, 2017
@omnidan
Copy link
Owner

omnidan commented Jul 10, 2017

published 1.7.0 with this merged

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.

None yet

6 participants