joypixels / emojify.js

A Javascript module to convert Emoji keywords to images
http://hassankhan.github.io/emojify.js/
MIT License
1.8k stars 238 forks source link

Feature parity between the browser and Node.js #94

Closed adam-lynch closed 10 years ago

adam-lynch commented 10 years ago

I'm creating this task since I didn't see a proper one for it.


So, we decided already that everything supported in the browser should also work when using the module with Node. Right now only .replace is supported.There are loads of issues and PRs related to this. This would also fix other issues like the bug where the emoji isn't found in <p>:)</p>.

I've already added a few tests for Node usage. We need to write tests for all features. Or even better, have tests that can easily be ran in either environment.

My idea for the implementation was to do everything with Node in mind first (i.e. just operating on strings). So using cheerio (or something like it) to do all the element type checking, etc. and then use whatever logic is used in .replace on any text. Then we'd compile it into one JS files for browser usage using browserify / webpack / an alternative.

@trevorah has split the parsing off into a separate module (see #93). He has an example using jsdom. I assumed we'd go with cheerio instead of jsdom to begin with because I heard jsdom is v heavy and cheerio has a nice API. Correct me if I'm wrong.

But #93 got me thinking anyway, if we do this, what happens in the browser when an emoji is found? Is the element re-created? (Maybe we'd have to take the root element, work on any text and then in the end we'd have a string again so we'd have to replace the root element?) Would the user lose their event handlers bound to the element? So, does it even make sense to use jsdom / cheerio at all?

Maybe we should have an API that underneath uses cheerio (for Node) / jQuery (for the browser) or something like that? Maybe jsdom does something like this already and I just don't know about it.


To do

trevorah commented 10 years ago

The use cases ive seen for node is that you either want to emojify a string as part of a template, or emojify a something like a blog post. The blog post is tricky as you would have to parse html without a dom.

The use cases ive seen for the browser are pretty similar, except that you have a dom for html parsing but you need to keep the size of the library small.

The "emojfy a string" use case is simple to solve for both node and browser, but the "blog post" use case (which requires html parsing) is difficult. We have to 1) have a dom parser for node and 2) keep the library small for browsers.

I think that the easiest way for us to support both, is to not include jsdom / cheerio / jquery as a dependency, and instead have clear instructions showing that you have to pass in a DomElement, and for node that means that the user has to use something like jsdom to create a DomElement. We have to use a DomElement for the browser anyway because of the points you mentioned (not destroying existing bound events etc).

adam-lynch commented 10 years ago

In my case, we're using browserify for our project so we're working in Node effectively but it'll be used in Node and most importantly the browser.

I think that the easiest way for us to support both, is to not include jsdom / cheerio / jquery as a dependency, and instead have clear instructions showing that you have to pass in a DomElement, and for node that means that the user has to use something like jsdom to create a DomElement. We have to use a DomElement for the browser anyway because of the points you mentioned (not destroying existing bound events etc).

I think the closer the implementation (& API) for both environments, the better. So are you saying the implementation would be the same? In Node, the user would just pass a DomElement created from jsdom and it would treated the same?

For use cases like mine where something could be used in Node or the browser, I guess the API should be the exact same so I could check which environment I'm in and choose whether to pass a real DomElement or one created via jsdom.

trevorah commented 10 years ago

Yep, if the implementation is the same for both node and browser, then we have less to test/break.

hassankhan commented 10 years ago

I agree, passing a DomElement would be ideal

4ver commented 10 years ago

@hassankhan My pr (https://github.com/hassankhan/emojify.js/pull/99) should resolve this. :smile:

All browser based tests are also run in node on my fork. Here's an example of how you use it in node: https://github.com/4ver/emojify.js/tree/develop#nodejs

hassankhan commented 10 years ago

I think @4ver's PR satisfies the two checkboxes in the first post, anyone else care to share their thoughts?

adam-lynch commented 10 years ago

Don't have time to check but I bet @4ver knows what he's doing and if the tests pass in Node, then who can argue :smile:

hassankhan commented 10 years ago

I believe this issue's ready to be closed, since I've merged #99