mtennoe / swagger-typescript-codegen

A Swagger Codegenerator tailored for typescript.
Apache License 2.0
140 stars 52 forks source link

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

Open itamarco opened 4 years ago

itamarco commented 4 years ago

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});
erictuvesson commented 4 years ago

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 commented 4 years ago

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

mtennoe commented 4 years ago

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 commented 4 years ago

I'm generating 2.0 swagger spec

mtennoe commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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