node-emoji
node-emoji copied to clipboard
omnidan
v2 Refactor
Signed-off-by: Richie Bendall [email protected]
Closes #55, Closes #57, Closes #62, Closes #66, Closes #77
@omnidan Looks like you need to remove Circle CI from trying to build this repository.
@Richienb I removed Circle CI. However, Travis is failing as well due to code style problems. Please adjust the code style according to the current standard or switch to http://standardjs.com, similarly to what omnidan/redux-undo uses.
@Richienb thanks! CI is passing now!
can you please check if we really need spdx-correct and validate-npm-package-license as dependencies (they might be subdependencies of other packages)?
These two packages seem to be GPL-3.0 licensed (according to fossa), which might "infect" our package with GPL-3.0 as well and discourage people from using it.
@omnidan It's not. Those packages are licensed under Apache 2.0 which allows distribution under MIT as long as we reference them which I'm pretty sure is done "automatically" via npm install where it's specified in a node_modules folder.
@Richienb alright, it might be fossa not detecting the license properly then. If they are Apache licensed it should be fine.
I have resolved the issues manually on fossa, I think it will update on the next commit
In v2, we've switched to using the official Unicode emoji names from https://unicode.org/Public/emoji/12.0/emoji-test.txt however, we could follow another list if need be.
Just putting my 2 cents in regarding this refactor.
I feel as though the use of lodash is unnecessary as it's use in this refactor can be replaced by simple native methods. Though it may not make much of a difference when it comes down to performance, I personally don't see the need to use an external library for things that can already be done easily using native JavaScript.
Additionally, is the CLI implementation in v2 really needed? I feel as though it's inclusion somewhat takes away from it being a simple library that provides emoji support for Node.js projects. Though usable, I feel most people who would download the package would not end up using it for its CLI component, and even if they did want what that provides it could be easily implemented anyways.
Other than that, good work on the refactor!
It doesn't look like emoji.json is being pushed to this v2 branch. Might want to look into that @Richienb.
@ShadowRanger emoji.json as in https://www.npmjs.com/package/emoji.json
The current version of this library comes with an emoji.json (https://github.com/omnidan/node-emoji/blob/master/lib/emoji.json) file, containing all of the data for the available emojis that the library can access. Your refactor seems to be missing this, and is requiring a non-existent file (https://github.com/omnidan/node-emoji/blob/v2/index.js#L5). How you want to implement this file is entirely up to you. You could add it to the library itself like is done in the current version, or you could use a dependency specifically for it like the one you linked. Just be sure that the library accesses the JSON data correctly.
Let me know if you would like me to elaborate on anything I have said, or if you have any trouble with it. Happy to help.
@Richienb My bad! I completely missed that. I have no idea how I did not see that in the package.json. I could have sworn that I checked first last night, clearly I did not have a clear state of mind at the time. My apologies!
@Richienb Just realised that I was looking at the v2 branch of this repository, rather than all of your latest changes in this pull request from your cloned repository. Either way, still my bad for not checking the right thing! Just wanted to clarify myself.
Mind if I ask, what's the state of this PR?
We're almost there. Just gotta do some cleanup.
What is the tradeoff we are willing to make in terms of dependencies vs extra code?
@Richienb it depends. If we only use one or two functions and the library is huge, I would rather add the extra code. I think in general node-emoji is so simple that we should try not to use dependencies if possible.
@Richienb it depends. If we only use one or two functions and the library is huge, I would rather add the extra code. I think in general node-emoji is so simple that we should try not to use dependencies if possible.
In this case, we're using lots of really small dependencies.