search

keycloak / keycloak-nodejs-connect

Apache License 2.0
676 stars 419 forks source link

Update TypeScript type definitions #301

Open Fydon opened 2 years ago

Fydon commented 2 years ago

Description

The TypeScript type definitions are out of sync with the JavaScript code which can make it difficult to use, especially for new users. Ideally these should be updated to be in sync with JavaScript and any contributions should require updating these.

This was previously raised in Jira: KEYCLOAK-13841 and KEYCLOAK-14578.

Discussion

No response

Motivation

TypeScript is very popular and the community has been trying to update these type definitions (#245, #250, #289, #290, #291), but most require review, especially given that there are a few now.

Details

No response

abstractj commented 2 years ago

@stianst @Fydon long time ago we discussed the idea of supporting Typescript, but I'm no expert on this. I'm supportive to have Typescript support for the Node.js adapter, as long as we have enough people to review. That means getting some help from @ssilvert @jonkoops @edewit @agagancarczyk

jonkoops commented 2 years ago

Yeah, it's an issue that these typings are no longer up-to-date. I think we're due for re-writing the source code in TypeScript so that this cannot happen in the future, but this might be something that we need to factor into a larger effort to re-write the code behind our adapters.

@abstractj I have no problem diving in to check out the work that needs to be done for this, but for the time being I am occupied with my work for the new admin ui.

abstractj commented 2 years ago

@jonkoops the only work necessary for now is related to review PRs with typescript definitions. If that's ok, next year I will request your team's input on some of them.

edewit commented 2 years ago

I'm up to the task to do some reviews on stuff like that

abstractj commented 2 years ago

@Fydon if you would like to contribute and update the typescript definitions, you are more than welcome. We should now have reviewers for this.

nbolten commented 2 years ago

Would reviewers be interested in taking a look at #290? It is mostly complete. Alternatively, I'd be ready to take #290 and update it with any changes suggested by reviewers or @Fydon.

nbolten commented 2 years ago

I would also be ready to translate the whole library to TypeScript-first, generating a JS library + .d.ts for distribution. But I want to check whether there would be reviewers and timely acceptance of a good pull request before doing that translation (since there are at least four TS type definition pull requests with no action).

jonkoops commented 2 years ago

@nbolten I agree that TypeScript source would be better and I would welcome any efforts in that respect. I am going to do some work to see what it would take to get it into the project.

helio-frota commented 2 years ago

I have a little xp in TS atm, and I'm not part of the KC team, in other words (just a personal opinion here), but when looking at the overall situation:

IMO, moving to TS is more healthy for the KC team and contributors.

jonkoops commented 2 years ago

Yeah, we need a strategy on how to adopt it incrementally as I don't believe in a complete re-write just to accommodate the types. I am working on small quality of life stuff first, after which we can start swapping out tooling such linters and compilers with something more TypeScript friendly.

After that I think it makes sense to convert the code in small chunks from JavaScript to TypeScript.

Fydon commented 2 years ago

I assume that this is the NodeJS library that is now deprecated? Any discussion about the deprecation should probably be made in discussions, but I thought I'd mention this here in case anyone was trying to update the Typescript definitions.