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

date-time format field is generated as string type #115

Open
itamarco opened this issue Mar 11, 2020 · 9 comments
Open

date-time format field is generated as string type #115

itamarco opened this issue Mar 11, 2020 · 9 comments
Labels

Comments

@itamarco
Copy link

Suppose we have a Date query param

@Get()
getNextDate(@Query('date') date: Date) { .. }

A generated swagger parameter definition for query param would be

"parameters": [
	{
		"in": "query",
		"name": "",
		"required": true,
		"format": "date-time",
		"type": "string"
	}
]

But swagger-typescript-codegen would type it as string

GetNextDate(parameters: {
        'date': string,
    } & CommonRequestOptions)

And when try to use it with Date, TS error would be raised Type 'Date' is not assignable to type 'string'
e.g.

const date:Date =  new Date();
const res = await cl.GetDate({date: date});
@ghost ghost added the Needs: triage 🔍 label Mar 11, 2020
@erictuvesson
Copy link

date-time is a string in the Swagger specs
https://swagger.io/docs/specification/data-models/data-types/#string

We should update the Typescript Type based on the format too, then convert that to string before the request.

I wonder if this could introduce a few breaking changes changing the type.

@itamarco
Copy link
Author

itamarco commented Mar 11, 2020

P.S. official swagger-codegen is indeed generating Date type from date-format (for typescript-fetch language)

@mtennoe
Copy link
Owner

mtennoe commented Mar 11, 2020

This code-generator currently only supports Swagger 2.0 right now, so we need to look at the spec for 2.0 here instead. Interestingly enough dates are not mentioned explicitly for 2.0, and looking more at the documentation it only briefly mentions some date examples where they are also passed as strings. So seems like the 2.0 spec doesn't consider dates at all. Not sure if that means we should add support for it ourselves, or fall back to strings.

@itamarco - Are you generating a 2.0 or 3.0 swagger spec in your example?

@itamarco
Copy link
Author

I'm generating 2.0 swagger spec

@mtennoe
Copy link
Owner

mtennoe commented Mar 11, 2020

Thanks!
Backwards compatibility is important so we could consider a solution that exposes a union type for dates, where the type would be Date | string, and then we could check for which type is being passed in from the caller side. The actual call would contain a string version of the date either way. This would maintain backwards compatibility, while making it easier for callers/consumers as well, where you wouldn't have to manually do that stringification of the date

Thoughts?

@itamarco
Copy link
Author

Sound good.

By union it with string we lose the date type enforcement but this is a price to pay for backward compatibility :(

Appreciate your fast response!

@mtennoe
Copy link
Owner

mtennoe commented Mar 11, 2020

Cool!
Not sure when a solution can be looked into though (my bandwidth is sadly filled at the moment), but anyone should be able to give it a shot :)

@erictuvesson
Copy link

By union it with string we lose the date type enforcement but this is a price to pay for backward compatibility :(

I suppose there could be an option to not have the backward compatibility, I would be interested in something like that.

@mtennoe
Copy link
Owner

mtennoe commented Mar 11, 2020

We could consider doing the string union as a backwards compatible stop-gap solution, and then remove the | string part when going to the next major version

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

No branches or pull requests

3 participants