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

Add Typescript types #106

Closed
tordans opened this issue Apr 5, 2022 · 8 comments
Closed

Add Typescript types #106

tordans opened this issue Apr 5, 2022 · 8 comments

Comments

@tordans
Copy link
Contributor

tordans commented Apr 5, 2022

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

Probably via DefinitelyTyped/DefinitelyTyped#59705 since this Repo is not written in TS.

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

@tordans
Copy link
Contributor Author

tordans commented Apr 6, 2022

MysteryBlokHed created on Typescript Types for itemsjs at DefinitelyTyped/DefinitelyTyped#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
Copy link
Member

cigolpl commented Apr 6, 2022

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
Copy link

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
Copy link
Contributor Author

tordans commented Apr 7, 2022

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 #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 289d8ba 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
Copy link
Member

cigolpl commented Apr 7, 2022

@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
Copy link
Contributor

Ruitjes commented Apr 8, 2022

@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
Copy link

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

@stereobooster
Copy link
Contributor

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

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

No branches or pull requests

5 participants