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

Make Optional API Fields Optional in Typescript Client #42

Open
dan-barrett opened this issue Dec 22, 2020 · 17 comments
Open

Make Optional API Fields Optional in Typescript Client #42

dan-barrett opened this issue Dec 22, 2020 · 17 comments
Labels
bug Something isn't working

Comments

@dan-barrett
Copy link

createUser(userId: UUID, request: UserRequest): Promise<ClientResponse<UserResponse>> {

userId I think should be an optional field here. The API and the docs say it is optional, but the Typescript client is forcing me to put one in.

There are other instances in this file where optional parameters are required.

@mooreds
Copy link
Contributor

mooreds commented Dec 22, 2020

Does it force the userId to be a valid UUID, or can you use null?

@dan-barrett
Copy link
Author

UUID is just a type alias for a string so neither null nor undefined work.

Argument of type 'null' is not assignable to parameter of type 'string'.ts(2345)

I will test with an empty string to see if that behaves the way I need, and worst case scenario I will generate a UUID client side in the short term. But still probably should be changed at some point.

@mooreds
Copy link
Contributor

mooreds commented Dec 22, 2020

Agreed, this appears to be a bug.

@robotdan robotdan added the bug Something isn't working label Dec 24, 2020
@facundorodriguez0
Copy link

facundorodriguez0 commented Jul 14, 2021

Hello! Hope you're doing well! I'm having the same issue here
image

Any updates about it?

@robotdan
Copy link
Member

Not sure the best way to fix this.

The current method signatures is:

register(userId: UUID, request: RegistrationRequest): Promise<ClientResponse<RegistrationResponse>> {

It should be (I think):

register(userId?: UUID, request: RegistrationRequest): Promise<ClientResponse<RegistrationResponse>> {

But we can't have a required parameter follow an optional, so would need to re-order these parameters to use the ? syntax. This would be a break for anyone using this library currently.

Is there a better way to do this in TS w/out re-ordering the parameters? Maybe we default the parameter to null when optional? This code looks to compile ok.

register(userId: UUID = null, request: RegistrationRequest): Promise<ClientResponse<RegistrationResponse>> {

@mooreds
Copy link
Contributor

mooreds commented Jul 23, 2021

If we use

register(userId: UUID = null, request: RegistrationRequest): Promise<ClientResponse<RegistrationResponse>> {

Do we need a way to mark UUID as optional (when building the client lib, not at runtime)?

If so, I don't think we have one, but we need such an attribute for openAPI (see https://github.com/FusionAuth/fusionauth-client-builder/blob/openapi-poc/src/main/api/createUserAction.json where I added required: false as an attribute).

@robotdan
Copy link
Member

We do have some "optional" capability. I think we currently parse for "Optional". We could make it more official in our DSL.
https://github.com/FusionAuth/fusionauth-client-builder/blob/947bc46da306d6819a7f537c74bd952c9a6c78a6/src/main/client/_macros.ftl#L226

@mooreds
Copy link
Contributor

mooreds commented Jul 29, 2021

Having a separate field in the DSL might be a little more ... robust ... than parsing a comment for the word 'Optional'. :)

@DanielJoyce
Copy link

You need to turn on 'strict' in the tsconfig.json, otherwise a whole bunch of useful checks are skipped and you get inconsistencies like this.

If the issue is browser vs node client, then splitting into seperate types, and sharing commong code is best option.

So ServerOptions, BrowserOptions both extend Options, etc.

I just fought this same mess with the Okta client.

@robotdan
Copy link
Member

robotdan commented Aug 4, 2021

Having a separate field in the DSL might be a little more ... robust ... than parsing a comment for the word 'Optional'. :)

Totally agree. This shouldn't be too hard to add.

@robotdan
Copy link
Member

robotdan commented Aug 4, 2021

Thanks for the feedback @DanielJoyce - I apologize we are bit weak on TypeScript over here.

In your experience, do clients built with OpenAPI / Swagger work pretty well? We have been reviewing options if we want to keep building our own template for these and battle with not knowing every language as well as we'd like, or if we can use something like OpenAPI to build a idiomatic client if we just build the correct DSL.

Thoughts?

@DanielJoyce
Copy link

Initially I'd stick to best practices for each use case.

For SPA, ocid using pkce. Redirect to service hosted form or hosted iframe, support authorization code and refresh token. Document needs around third party cookies, configuring custom domains to handle cors issues, etc. This would apply to authfusion and okta along with others as well. You can support your own service explicitly and ask for community help to document the others. Make the JS lib not depend on any frameworks.

For server side I think implicit flow may be the reccomended solution. I'd have to check.

Try to design these systems as the bare necessity core. Then you can base other things on top. Once you have a JS lib you can use it for react and vue or whatever support. Once you have a python lib, layer on flask or whatever. Same for java, etc.

If the core abstraction is good then it can be used wherever it is needed. The community could easily provide it.

@akoskm
Copy link

akoskm commented Jul 10, 2022

@robotdan any updates on this? register's first parameter is also optional in the docs but it's not in the actual types:

* @param {UUID} userId (Optional) The Id of the user being registered for the application and optionally created.
* @param {RegistrationRequest} request The request that optionally contains the User and must contain the UserRegistration.
* @returns {Promise<ClientResponse<RegistrationResponse>>}
*/
register(userId: UUID, request: RegistrationRequest): Promise<ClientResponse<RegistrationResponse>> {
return this.start<RegistrationResponse, Errors>()
.withUri('/api/user/registration')
.withUriSegment(userId)
.withJSONBody(request)
.withMethod("POST")
.go();
}

@nfebe
Copy link

nfebe commented Jun 20, 2023

I think this problem is widespread across the library?

For example : exchangeRefreshTokenForAccessToken referenced below marks all parameters in the function documentation as optional but does nothing to enforce such optionality. As a result, the method is not callable (without errors) except all parameters are provided.

I am wondering if I have missed something or this miss is just as basic as it looks?

exchangeRefreshTokenForAccessToken(refresh_token: string, client_id: string, client_secret: string, scope: string, user_code: string): Promise<ClientResponse<AccessToken>> {

@slifty
Copy link

slifty commented Aug 24, 2023

This seems like something that should be fixed fairly urgently, as it forces typescript users to pass invalid data to essentially all methods. Is this project being maintained?

@mooreds
Copy link
Contributor

mooreds commented Aug 29, 2023

Thanks @slifty. The project is being maintained :) . We had an internal discussion but don't have a timeline for you for this issue.

@steven-t-h
Copy link

Checking on this, still an issue four years later. What is the suggested workaround while we must deal with a broken library? For registration should I just pass an empty string? Or do I need to force the library to accept a null for userId myself?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

9 participants