emojify-parser proof of concept

[trevorah]

DO NOT MERGE THIS

This is a proof of concept that uses emojify-parser, so that the emojify function only has to worry about dom manipulation.

To use with nodejs, you would have to use jsdom like this:

jsdom.env('<span id="cat">hello :cat:</span>', function(err, window) {
  var el = window.document.getElementById('cat');
  emojify(el, { document: window.document });

  console.log(el.outerHTML)
  // '<span id="cat">hello <img class="emoji" src="/cat.png" title=":cat:" alt=":cat:" align="absmiddle"></span>'
});

Let me know what you think and whether or not I should carry on and make this fit for merging.

[adam-lynch]

So is emoji-parser the same code that was in emojify.js previously? And what about stuff like emojify.replace?

[trevorah]

Not quite, the previous code uses a regular expression to replace the :emoji: with the matching image tag. By separating out the emoji parser from the part that replaces/inserts the image tag, we can test the individual parts and have something a lot less buggy. Its kind of like separating out the parts of a compiler.

Youre right about emojify.replace, I should probably switch to constructing the html with strings rather than depending on document.createElement. We could also reuse emojify.replace internally as long as we only use it on nodes where the type is TEXT_NODE, that way we can guarantee that there will be no html in the string that we are calling emojify.replace with.

[adam-lynch]

I just read over the jsdom README and this again.

Youre right about emojify.replace, I should probably switch to constructing the html with strings rather than depending on document.createElement

What's wrong with that if options.document is being passed?

[trevorah]

I guess its just overkill to require a dom to concatenate strings if youre using emojify.replace. Especially when running in node.

[adam-lynch]

But we'd have it anyway right? We'd require it for traversing so why not use it? So it could be safer? Maybe I'm missing something.

[trevorah]

would you traverse for emojify.replace?

[adam-lynch]

Ha sorry 😝🔨. I was thrown off by thinking of my own use case; I'm only using .replace because the other features aren't supported in Node so I wouldn't be using .replace once this is finished.

[trevorah]

haha, no worries.

[hassankhan]

Does this have any appreciable effect performance-wise? I would think not, but it would be nice to know? I'll admit, I was a bit against it to begin with but I'm slowly warming to the idea.

[trevorah]

I cant see it having a massive effect. Tree traversal would cause the biggest change in performance, but thats already happened in #99.

[hassankhan]

Alrighty then, I think it's the right thing to do, looking forward to seeing some more commits 😄