Add flow support - Githubissues

zzarcon / gh-emoji

Github emoji parsing done right :point_up: :pray: :point_up_2: :clap: :ok_hand:

http://zzarcon.github.io/gh-emoji/

MIT License

160 stars 7 forks source link

Add flow support #14

Closed ryyppy closed 8 years ago

ryyppy commented 8 years ago

See commit messages for more info

ryyppy commented 8 years ago

Solves issue #5

zzarcon commented 8 years ago

@oskarcieslik take a look ;)

rpunkfu commented 8 years ago

@zzarcon checking :)

rpunkfu commented 8 years ago

Hi @ryyppy! I checked this PR and my thoughts are:

please remove all "//eslint-disable-line" comments from files and instead use solutions I proposed or if you have your own idea how to solve it, I'd love to hear it :)
file gh-emoji.js.flow shouldn't go into dist folder since it's only for transpiled / minified .js code to be used in browser by putting it in tag. Instead please create folder lib / src and put it there. (Maybe we should create flow sub-folder for flow interfaces, declarations, etc?)
I don't know what types-test.js is supposed to do, can you explain it to me? :)

Thanks @ryyppy :)

ryyppy commented 8 years ago

Cool, thanks for the instant review :-)

So for your points:

I agree with //eslint-disable-line and will rename it to emojiMap
I also agree for the dist/gh-emoji.js.flow... but if we set dist/gh-emoji.js as main, we need to make sure the js.flow file is in the same directory, otherwise flowtype won't pick up the definitions... I have two proposals how we could settle this:
- Add a cp src/gh-emoji.js.flow dist in the dist script (probably the best solution for now)
- Or put the declaration file as independent module declaration in the (newly official) flowtype repo (https://github.com/flowtype/flow-typed) - I will do that eventually, when this lib is stable
types-test.js is basically a test file you can open, just to see if flowtype picks up the js.flow definitions correctly... this will be important for tests in the flow-typed project

Let me know what you think!

zzarcon commented 8 years ago

+1 for the eslint and for adding cp src/gh-emoji.js.flow into the dist npm script.

Should be ready to merge after those changes :muscle:

rpunkfu commented 8 years ago

I think cp src/gh-emoji.js.flow dist is fine! :) How about having two directories bin and dist, where:

dist will be for user who wants to manually download script and put it in script tag
bin will be for npm's starting point

This way we can cp src/gh-emoji.js.flow bin and user will not be confused about .flow file in dist

ryyppy commented 8 years ago

By separating js.flow files into another directory, this will force flowtype users to add another [lib] path to their .flowconfig.

So they end up with another installation step (e.g. with following .flowconfig):

[lib]
interfaces/
node_modules/gh-emoji/bin/gh-emoji.js.flow

Do you think this is a huge problem for webdevs to deal with this extra files? Usually you also pack stuff into dist like .js.map or .min.js and they cherry pick stuff either via bower or via node? :-)

rpunkfu commented 8 years ago

Yeah, you're right :)

ryyppy commented 8 years ago

To summarize and make this PR work I need to do following:

[x] Pull merge master (since functionality was added and this functionality is not typed yet)
[x] Remove //eslint-disable-line stuff
[x] Move the flow.js into a directory called definitions
[x] Add the cp definitions/gh-emoji.js.flow dist to thedist command
[ ] ~~Add the flow check gh-emoji.js to the dist command~~ (will postpone this for now)

rpunkfu commented 8 years ago

Yes, that's right :)

zzarcon commented 8 years ago

@ryyppy looks good for me 👍

rpunkfu commented 8 years ago

same here :)