Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

migrated the project to typescript #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wvanderp
Copy link

@wvanderp wvanderp commented Oct 9, 2021

When I was looking for a lib to diff my array of objects, I stumbled on this project. I liked the simplicity of the API. I wanted to use it, but the problem was that there were no typescript types available.

So my quest was to write typescript types for the project and submit those. But then I saw that the code was a bit more complex. So to understand the code and to automatically generate types, I started translating the project into TS.

Then the tests failed, so I had to fix those and translate them to TS. Then eslint started complaining about not knowing typescript, so I also fixed that.

And finally, I thought that since most of the functions of lodash could be replaced by vanilla javascript, I tried to remove lodash to make the package smaller and less vulnerable.

So to summerise, this PR contains:

  • Translation to TS
  • TS types
  • Removed lodash
  • Extended unit tests
  • Refactored code

I know that it is a bit much to drop this all in one PR. and I don’t know if you want to change this repo over to typescript.
If you want only one or two of the above-mentioned changes, I’m happy to split them out for you.

I am available to answer any questions you may have, and I am happy to implement any feedback.

With kind regards,

Wvdp

PS. I am participating in hacktoberfest, so for this PR to count towards that, would you be as kind to add the hacktoberfest-accepted to this PR.

@malcolmvr
Copy link
Owner

Good effort here @wvanderp. I'm unfamiliar with Typescript and prefer FP over OOP so I'd prefer to keep the repo as-is.
You had a good thought about reducing dependency on other packages (lodash). My goal was for there to be as little code in this repo as possible and to rely on common packages that already receive a lot of testing. It's a trade-off, to be sure, but in this case I think less (code) is more.

@wvanderp
Copy link
Author

Typescript adds static code type checking to javascript by adding some extra code features.
This youtube video has a short explanation: https://www.youtube.com/watch?v=zQnBQ4tB3ZA

A big disadvantage of typescript is that every module that you call in typescript also needs types otherwise the types can be checked. So this package is less useful to people using typescript because the type checking stops when using this module.

There are two ways of adding typescript to a repo. The first way is by writing it completely in typescript. The second way is by defining the types of the publicly facing interfaces in a separate file.

I like this package now that I can use it in my project (using my own fork) and I’d like to share that with others.

What I will do is remove the typescript stuff and most of the other changes. But I add in the types file that will achieve the same result.

Also since this project is not compiled or repacked I advise using the individual wrapped packages of lodash to cut down on the file size.

@wvanderp
Copy link
Author

I limited the changes to a smaller scope:

Added .mocharc.json
Deleted test/mocha.opts
Mocha.opts is deprecated. so I replaced it with the modern equivalent

Added lib/index.d.ts
This file contains the typescript types

Modified lib/index.js
Both getKey and groupingFunction contained side effect. I replaced those by calculating them directly.
Replaced the difficult to read if statement tree with a switch statement.

Modified package-lock.json
Removing the package-lock.json file allows npm to chose up to date packages.

Modified package.json
Added the types key to notify both npm and typescript where to find the types

Added test/both.spec.js, test/customComapair.spec.js, test/deep.spec.js, test/errors.spec.js, test/first.spec.js, test/second.spec.js
Deleted test/index.js
I split out the test and added all test cases for all return types. So now we check all the return types for all the test cases we only used on updatedValues.first.

My original offer still stands. If you would like to see any changes or clarifications before considering this PR please ask.

-wvdp

@grindpride
Copy link

Hey @malcolmvr
Can you merge this please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants