Skip to content

Fix implementation to avoid Object.prototype mutation, + perf improvement#47

Merged
omnidan merged 3 commits into
omnidan:masterfrom
timruffles:fix
Jul 10, 2017
Merged

Fix implementation to avoid Object.prototype mutation, + perf improvement#47
omnidan merged 3 commits into
omnidan:masterfrom
timruffles:fix

Conversation

@timruffles

@timruffles timruffles commented Jul 7, 2017

Copy link
Copy Markdown
Contributor
  • 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
Copy Markdown
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

smeijer commented Jul 7, 2017

Copy link
Copy Markdown

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

Comment thread lib/emoji.js
*/
Emoji.unemojify = function unemojify(str) {
if (!str) return '';
var words = toArray(str);

Copy link
Copy Markdown

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

@bbrzoska bbrzoska Jul 7, 2017

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown

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!

@bbrzoska bbrzoska left a comment

Copy link
Copy Markdown

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.

Comment thread lib/emoji.js
*/
Emoji.unemojify = function unemojify(str) {
if (!str) return '';
var words = toArray(str);

@bbrzoska bbrzoska Jul 7, 2017

Copy link
Copy Markdown

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

omnidan commented Jul 10, 2017

Copy link
Copy Markdown
Owner

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

omnidan commented Jul 10, 2017

Copy link
Copy Markdown
Owner

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.

6 participants