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

Doesn't work with Node #50

Closed adam-lynch closed 10 years ago

adam-lynch commented 10 years ago

emojify.js#L19 breaks it since there's no window. I'm trying to just use emojify.replace (and of course emojify.setConfig).

I would create a PR and change that line to something like var document = typeof window !== 'undefined' && window.document;. It works. But I guess you'd want it to be more sophisticated than that? i.e. if window doesn't exist, they should only be able to do certain things (it should never try to call document.createElement).

Related: #39

hassankhan commented 10 years ago

Yep thanks for creating the issue, I recently noticed the same myself actually.

adam-lynch commented 10 years ago

I don't mind helping, but I'm not sure what way you want to go with it. I wonder could you use something like cheerio so then you could even support things like ignoring certain element tag names, etc. even when using with node.

hassankhan commented 10 years ago

This is sort of one of the reasons I never really thought about Node when I wrote this, I'm not quite sure how it would work. I was sort of hoping that it would just work by itself, but it does not unfortunately. Maybe other contributors might have better ideas/solutions? @qq99 @suprememoocow

qq99 commented 10 years ago

Can you still use the string version of emojify?

adam-lynch commented 10 years ago

@qq99 No, see my first comment here. If I change that line, then yes I can use .replace.

qq99 commented 10 years ago

Is that line really necessary? Won't referencing document automatically look at window.document? If it must be there, I'd probably just wrap it in an if (window) { ... } block

adam-lynch commented 10 years ago

That's basically what my line does. That would be fine and maybe should be done as a quick fix, but it might be worth considering providing all features to Node users via cheerio or something.

qq99 commented 10 years ago

I'd be OK with it being a documented note: "If you are running this on the server, you can only use _____" type thing in the README. As a user, I wouldn't expect the DOM related manipulations to work on the server, so I think it's really up to @hassankhan's preference

hassankhan commented 10 years ago

I never really quite got the use-case of running it on the server, and as such it was never a priority IMO. However, I'm looking at moving emojify.js to 1.0 and continually want to make it useful, so I'm not ruling it out.

For now, I'm in agreement with @qq99, using .replace() would be the way to go on the server. Let's just get it working on Node for now. In a future release, we can definitely improve on it.

@adam-lynch: If you'd kindly make a PR, I'll merge.

adam-lynch commented 10 years ago

@hassankhan no prob

adam-lynch commented 10 years ago

Ok, see above. I wonder is there a need for a separate set of tests for Node, since the existing tests are ran in the browser.

hassankhan commented 10 years ago

Closing in favour of #67