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

[KEYCLOAK-14578] Update KeycloakConfig types #250

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

beevelop
Copy link

Some Types are missing in the current implementation.
Related Jira-Issue: https://issues.redhat.com/browse/KEYCLOAK-14578

@ssilvert ssilvert self-assigned this Jun 26, 2020
Copy link

@abstractj abstractj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@beevelop it seems somewhat related to the stale PR here #245. Maybe we could solve it in a single PR? If you would like to do that, I'd suggest to cherry-pick the commit from the original author keeping the authorship and add your commit on top of that.

Otherwise, we can move forward with your PR as is. The issues on Travis seem unrelated to the changes here.

@beevelop
Copy link
Author

beevelop commented Jul 1, 2020

Looks like there are a few more changes in PR #245. So I'd say whatever works best for the reviewers. If the other changes are valid too, I can cherry-pick the respective commits into this PR.

@abstractj
Copy link

@beevelop if you are ok about this idea, let's do this. We cherry-pick changes from #245 keeping the authorship and update this pull-request with your commit. Thank you!

Copy link

@bruegth bruegth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should avoid using quotes around variables.

'clientId': string
'secret': string
'resource'?: string
'public-client'?: string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Type should be changed into boolean here:

'resource': string
'ssl-required': string
'realm': string
'clientId': string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'clientId': string
clientId: string

'realm': string
'clientId': string
'secret': string
'resource'?: string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'resource'?: string

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'resource' or 'client-id' see here:

this.clientId = resolveValue(config.resource || config['client-id'] || config.clientId);

'auth-server-url': string
'resource': string
'ssl-required': string
'realm': string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'realm': string
realm: string

'ssl-required': string
'realm': string
'clientId': string
'secret': string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'secret': string
secret: string

'bearer-only'?: boolean
realm: string
'plainKey'?: string
'verifyTokenAudience'?: boolean
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'verifyTokenAudience'?: boolean
verifyTokenAudience?: boolean

'bearer-only'?: boolean
realm: string
'plainKey'?: string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'plainKey'?: string
realmPublicKey?: string

'clientId': string
'secret': string
'resource'?: string
'public-client'?: string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'public-client'?: string
public?: string

@@ -18,12 +18,18 @@ interface KeycloakConnectStatic {
declare namespace KeycloakConnect {

interface KeycloakConfig {
'confidential-port': string|number
'auth-server-url': string
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'auth-server-url': string
authServerUrl: string

'public-client'?: string
'realmUrl'?: string
'realmAdminUrl'?: string
'minTimeBetweenJwksRequests'?: number
'bearer-only'?: boolean
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'bearer-only'?: boolean
bearerOnly?: boolean

@bruegth
Copy link

bruegth commented Dec 14, 2020

Typescript won't work with this package, maybe it's better to remove .d.ts completely until it's covered by unit tests.

@c0bra
Copy link

c0bra commented Dec 30, 2020

What's the workaround for using this package with typescript in the meantime?

@bruegth
Copy link

bruegth commented Dec 31, 2020

What's the workaround for using this package with typescript in the meantime?

I've created a gulp task to replace the definition file in the node_modules folder with a modified one.

@ItsTarik
Copy link

Hi and thanks for reporting,I've got an other similar issue! it's hard to find useful information about this issue since the typescript config KeycloakConfig is missing the realm-public-key prop already !
So that's what i get with both version ^11.0.3 and the actual one ^12.0.4:

  interface KeycloakConfig {
    'confidential-port': string|number
    'auth-server-url': string
    'resource': string
    'ssl-required': string
    'bearer-only'?: boolean
    realm: string
  }

So in my case i can use the missing declared property: 'realm-public-key': 'pemkey', while ignoring the ts checking.
I will create a separate issue for this but i think that's important to list in this thread since its similar to the issue subject.

@ffleandro
Copy link

What is the current status for Typescript support and what was the blocker on this PR?

@abstractj abstractj requested review from edewit and jonkoops January 12, 2022 22:35
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.

9 participants