v2 Refactor #86

Richienb · 2020-01-07T13:42:56Z

Signed-off-by: Richie Bendall richiebendall@gmail.com

Closes #55, Closes #57, Closes #62, Closes #66, Closes #77

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Richienb · 2020-01-07T13:47:04Z

@omnidan Looks like you need to remove Circle CI from trying to build this repository.

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

omnidan · 2020-01-08T11:25:29Z

@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.

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

omnidan · 2020-01-08T15:04:04Z

@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.

Richienb · 2020-01-08T20:24:31Z

@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.

omnidan · 2020-01-09T09:01:26Z

@Richienb alright, it might be fossa not detecting the license properly then. If they are Apache licensed it should be fine.

omnidan · 2020-01-09T09:03:05Z

I have resolved the issues manually on fossa, I think it will update on the next commit

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

…cores Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Richienb · 2020-01-11T11:49:28Z

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.

ShadowRanger · 2020-01-19T10:47:03Z

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!

… native methods. Signed-off-by: Richie Bendall <richiebendall@gmail.com>

ShadowRanger

Very fast code response to my suggestion! Looks good to me 👍

ShadowRanger · 2020-02-26T09:18:37Z

It doesn't look like emoji.json is being pushed to this v2 branch. Might want to look into that @Richienb.

Richienb · 2020-02-26T09:34:54Z

@ShadowRanger emoji.json as in https://www.npmjs.com/package/emoji.json

ShadowRanger · 2020-02-27T11:21:28Z

@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 · 2020-02-27T11:23:38Z

@ShadowRanger I'm actually requiring the npm module called emoji.json.

ShadowRanger · 2020-02-27T11:34:55Z

@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!

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

JeDaYoshi · 2020-09-12T00:22:57Z

Mind if I ask, what's the state of this PR?

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Richienb · 2020-11-17T05:12:28Z

Mind if I ask, what's the state of this PR?

We're almost there. Just gotta do some cleanup.

Richienb · 2021-01-26T09:18:23Z

What is the tradeoff we are willing to make in terms of dependencies vs extra code?

omnidan · 2021-01-26T09:27:05Z

@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 · 2021-01-26T09:29:23Z

@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.

JoshuaKGoldberg · 2021-11-18T16:21:03Z

package.json

+  "main": "index.js",
+  "files": [
+    "index.js",
+    "index.d.ts"


IME the three best ways to have TypeScript support are:

Write the original package in TypeScript

Include .d.ts files and tests in the repository

Defer to DefinitelyTyped for typings

Including raw .d.ts files in this source repo but not testing them means contributors+maintainers will all have to manually check them. That's kind of hard if you're not TypeScript-familiar yourself -- and if you are, then why not write in TypeScript? 😜

Proposal: remove index.d.ts and file a followup to discuss either rewriting in TypeScript or moving to DefinitelyTyped?

I'm for rewriting it in TypeScript if you are up for it - that would definitely be the best solution 😁

JoshuaKGoldberg · 2021-11-18T16:22:12Z

.gitattributes

@@ -0,0 +1,288 @@
+# Common settings that generally should always be used with your language specific settings


I haven't seen many repos include all these settings -.gitattributes, .gitignore, .. -- are they necessary in this PR?

I agree, we should only have necessary files, maybe @Richienb can elaborate a bit?

JoshuaKGoldberg · 2021-11-18T16:22:46Z

.editorconfig

+
+[*.yml]
+indent_style = space
+indent_size = 2


Does changing the formatting engine need to be in this PR? Seems like it's adding a lot.

Proposal: split it out so I can evangelize Prettier? 😄

Personally a fan of prettier as well, so I'm fine with doing it in a separate PR with prettier! 💅

I mean, I am personally up for it - so our editors know what way to follow.

JoshuaKGoldberg · 2021-11-18T16:24:34Z

.travis.yml

 node_js:
-  - "0.10"
+  - "lts/*"
+  - "node"


Doesn't have to be in this PR, but just dropping a note - I'm down to switch over to GitHub Actions. Haven't seen a .travis.yml update that wasn't a removal in quite a while 😄

JoshuaKGoldberg · 2021-11-18T16:29:31Z

package.json

+  },
+  "scripts": {
+    "lint": "standard",
+    "test": "standard && ava",


Suggested change

"test": "standard && ava",

"test": "ava",

Also, out of curiosity: any reason for ava? Nothing against it; I just haven't seen many projects using it.

I think back then I had some issues with jest, which was a loooong time ago though. I think now jest would be the more standard and stable solution for this 🃏

JoshuaKGoldberg · 2021-11-18T16:32:35Z

README.md

+
+Get an emoji name from an emoji.
+
+#### emoji


It's an accessibility oddity to have headings with the same name (and also linking then only works for one). Should look at getting each of these unique text values...

JoshuaKGoldberg · 2021-11-18T16:34:07Z

index.js

+const { assert } = is
+
+const emojiData = Object.entries(require('emojilib').lib).map(([name, { char: emoji }]) => [name, emoji])


I personally find it confusing to have requires nested within other statements. It'll also be harder to convert this to ESM if & when the time comes.

JoshuaKGoldberg · 2021-11-18T16:35:24Z

README.md


-## Installation
-To install `node-emoji`, you need [node.js](http://nodejs.org/) and [npm](https://github.com/npm/npm#super-easy-install). :rocket:
+## Usage


Now that emojis are being sourced from other libraries, it'd probably be good to link to those libraries in the README.md.

JoshuaKGoldberg · 2021-11-18T16:59:15Z

package.json

+    "index.d.ts"
+  ],
+  "engines": {
+    "node": ">=10"


Suggested change

"node": ">=10"

"node": ">=14"

any reason why we should support 14 and higher only?

Node 12 is now only in maintenance, and 14 is LTS. I suppose 12 would work too 🙂 but newer Node versions mean we can go with newer JS syntax.

I say better keep it on 10 unless it's really needed, or the new additions are just great enough to justify the compatibility breakage.

JoshuaKGoldberg · 2021-11-18T21:54:37Z

README.md

+##### removeSpaces
+
+Type: `boolean`\
+Default: `true`.


I've always found it confusing to have APIs that default a boolean parameter to true when not provided. Can we call this:

Suggested change

##### removeSpaces

Type: `boolean`\

Default: `true`.

##### preserveSpaces

Type: `boolean`\

Default: `false`.

JoshuaKGoldberg · 2021-11-19T08:41:48Z

index.js

+}
+
+exports.find = emoji => {


Before there were *ByCode and *ByName equivalents for APIs such as .find. Was it intentional to remove them in v2? (I can't find discussion)

@omnidan thoughts?

I would not know of a reason to remove them, maybe they were just forgotten about in the rewrite? /cc @Richienb

Added them back in #113.

Richienb added 3 commits January 7, 2020 23:15

chore: Ignore lockfiles

1907af9

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

refactor!: v2

b7e8195

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

chore: Specify proper node version

3494b41

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Richienb added 2 commits January 8, 2020 02:49

feat: Normalize key name

deeb1f1

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

feat: Add simple CLI

ca53e56

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

chore: Switch to standard

9088dd6

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Richienb added 3 commits January 9, 2020 23:34

fix: Catch invalid CLI commands

149710f

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

style: Fix lint

ab2e39c

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

fix: Allow specifying spaces which are inferred from dashes or unders…

b63a6ed

…cores Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Richienb marked this pull request as ready for review January 11, 2020 11:46

Richienb requested a review from omnidan January 11, 2020 11:46

Merge branch 'master' into v2

1b724c1

refactor!: Remove CLI for seperate implementation and swap lodash for…

bdcb913

… native methods. Signed-off-by: Richie Bendall <richiebendall@gmail.com>

ShadowRanger approved these changes Jan 19, 2020

View reviewed changes

Richienb and others added 3 commits January 20, 2020 21:26

docs: Fix build status badge

13f2414

Fix surrogate pairs parsing

3fa0ff4

fix: Properly handle UTF-16 surrogate pairs

95fc46b

Richienb added 6 commits July 11, 2020 14:05

Compact imports

c050aa8

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Replace emojilib with unicode-emoji-json

c9fd515

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Fix unit tests

f36e244

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Change key to name

29b8d36

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Replace ow with @sindresorhus/is

b9c9f3d

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Replace argument name string with input

97f910e

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Richienb added 3 commits November 17, 2020 18:06

Meta tweaks

7fd5cae

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Switch to emojilib and update dependencies

d32da6c

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

Update documentation

3b21bc2

Signed-off-by: Richie Bendall <richiebendall@gmail.com>

ceisele-r mentioned this pull request Jan 26, 2021

Create updated release with bumped dependencies #97

Closed

JoshuaKGoldberg reviewed Nov 18, 2021

View reviewed changes

JoshuaKGoldberg mentioned this pull request Nov 18, 2021

v2 Refactor, Refactored #113

Merged

JoshuaKGoldberg reviewed Nov 19, 2021

View reviewed changes

omnidan merged commit 3b21bc2 into master May 15, 2023

JoshuaKGoldberg mentioned this pull request May 15, 2023

Convert to TypeScript #127

Closed

JoshuaKGoldberg deleted the v2 branch May 15, 2023 23:36

		@@ -0,0 +1,288 @@
		# Common settings that generally should always be used with your language specific settings

		const { assert } = is

		const emojiData = Object.entries(require('emojilib').lib).map(([name, { char: emoji }]) => [name, emoji])

		}

		exports.find = emoji => {

v2 Refactor #86

v2 Refactor #86

Conversation

Richienb commented Jan 7, 2020 • edited Loading

Richienb commented Jan 7, 2020

omnidan commented Jan 8, 2020

omnidan commented Jan 8, 2020

Richienb commented Jan 8, 2020 • edited Loading

omnidan commented Jan 9, 2020

omnidan commented Jan 9, 2020

Richienb commented Jan 11, 2020

ShadowRanger commented Jan 19, 2020

ShadowRanger left a comment

Choose a reason for hiding this comment

ShadowRanger commented Feb 26, 2020

Richienb commented Feb 26, 2020

ShadowRanger commented Feb 27, 2020 • edited Loading

Richienb commented Feb 27, 2020

ShadowRanger commented Feb 27, 2020

JeDaYoshi commented Sep 12, 2020

Richienb commented Nov 17, 2020

Richienb commented Jan 26, 2021

omnidan commented Jan 26, 2021

Richienb commented Jan 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Richienb commented Jan 7, 2020 •

edited

Loading

Richienb commented Jan 8, 2020 •

edited

Loading

ShadowRanger commented Feb 27, 2020 •

edited

Loading