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

Update TypeScript type definitions #301

Open
Fydon opened this issue Nov 10, 2021 · 11 comments
Open

Update TypeScript type definitions #301

Fydon opened this issue Nov 10, 2021 · 11 comments

Comments

@Fydon
Copy link

Fydon commented Nov 10, 2021

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

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

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

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

edewit commented Dec 23, 2021

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

@abstractj
Copy link

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

@abstractj abstractj removed their assignment Dec 23, 2021
@nbolten
Copy link

nbolten commented Jan 6, 2022

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

nbolten commented Jan 6, 2022

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

jonkoops commented Jan 6, 2022

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

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:

  • keycloak-nodejs-admin-client - TS
  • keycloak-admin-ui - TS
  • keycloak-nodejs-connect - JS

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

@jonkoops
Copy link
Contributor

jonkoops commented Jan 21, 2022

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

Fydon commented Feb 8, 2022

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.

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

No branches or pull requests

6 participants