itemsapi / itemsjs

Extremely fast faceted search engine in JavaScript - lightweight, flexible, and simple to use
Apache License 2.0
346 stars 41 forks source link

Add Typescript types #106

Closed tordans closed 2 years ago

tordans commented 2 years ago

It would be great to have some shared Typescript types for ItemsJs.

Probably via https://github.com/DefinitelyTyped/DefinitelyTyped/discussions/59705 since this Repo is not written in TS.

Did anyone create types in their implementation of the library that they could share?

tordans commented 2 years ago

MysteryBlokHed created on Typescript Types for itemsjs at https://github.com/DefinitelyTyped/DefinitelyTyped/pull/59717. Maybe @cigolpl or @Ruitjes (*) could have a quick look at this as a review. I am too new to the itemsjs (and Typescript) to get all the details.

I could contribute the readme update to inform about the type definitions once they are on npm.

*) I hope the ping does not bother too much; it looks like you where last active here.

cigolpl commented 2 years ago

Tobias, thanks for your initiative regarding TS! I am not that experienced now in TS to review this..

Could you tell me the benefits of TypeScript in ItemsJS ? Is it to make ItemsJS codebase easier to maintain, or just to make easier, native integration with your current project ? I'd like to get more familiar with this

MysteryBlokHed commented 2 years ago

The benefits of TypeScript in this case are mostly for developers using itemsjs. The types will give them better IDE support with autocomplete and will highlight any errors such as incorrect types or missing arguments for functions.

tordans commented 2 years ago

In addition to what MysteryBlokHed said:

The great thing about DefinitelyTyped is, that it allows adding Types separate from the main project. So no need to change anything here.

But I would recommend editing the readme here to hint at those types.

And ideally relevant changes to the data structure here should be reported back to the DefinitelyTyped project so the Types can be updated as well.

@cigolpl Since you know the codebase and feature well, it be great if you could have a look at the types and see if you see any issues with the data structure that they represent. The types are a bit advanced (I could not write them like this myself…) and rely on each other, but a general check should be possible. I did a check just now and added few comments which you might help clarify. — Part of this is also https://github.com/itemsapi/itemsjs/pull/107 which holds the changes that I reference in my review comments.

Back to the "why TS" question: While doing the review I saw two interesting bit, the change from https://github.com/itemsapi/itemsjs/pull/107/commits/289d8ba2bd0f8a8efc06d6f3a7a8fb6dfbb2f169 would be spotted by a typed codebase since the naming did not match on the configuration.md. And the guard like https://github.com/itemsapi/itemsjs/blob/master/src/lib.js#L220-L222 would not be needed in a fully typed environment since TS already makes sure the name is always present.


Thanks for the great library! And the types!

cigolpl commented 2 years ago

@tordans I've taken the look at the review of DefinitelyTyped and in my opinion you are doing it really well!

If you implement the TS into ItemsJS - the current unit tests should tell you if implementation is correct and if application is working

If @Ruitjes has some time he would be invaluable here with TS because he created (with his team) Instant Search Adapter over ItemsJS in TypeScript (https://github.com/unplatform-io/instantsearch-itemsjs-adapter)

I'd like to make some improvement but currently I don't have a time resources. I'll get back to this project (and its c++ version) hopefully soon. I am also open for project "co-founding" / sharing ownership

Ruitjes commented 2 years ago

@tordans Thanks for adding DefinitelyTyped support for ItemsJS and fixing up some of the readme! 🎉 Unfortunately, I'm currently busy with my school projects so I don’t really have time to actively work to provide ItemsJS with Typescript. But, who knows maybe the near future.

MysteryBlokHed commented 2 years ago

The PR at DefinitelyTyped/DefinitelyTyped#59717 was merged yesterday, so types are now available on npm at @types/itemsjs 😄

stereobooster commented 10 months ago

In case if anybody is interested here is my attempt to fully port library to typescript https://github.com/stereobooster/itemsjs/tree/typescript