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

[Question] [typescript-fetch] The date format (RFC 3339: full-date) has a time zone gap when parsing from JSON and when converting to JSON #5932

Open
mdoi2 opened this issue Apr 14, 2020 · 4 comments

Comments

@mdoi2
Copy link

mdoi2 commented Apr 14, 2020

target: typescript-fetch
openapi-generator version: 4.3.0

For example, if we generate from the definition of requestBody like this:

requestBody:
  required: true
  content:
    application/json:
      schema:
        title: RequestBody
        type: object
        properties:
          date:
            type: string
            format: date
            nullable: true

Such a code will be generated in the models directory:

export interface RequestBody {
  date?: Date | null;
}

export function RequestBodyFromJSON(json: any): RequestBody {
  return RequestBodyFromJSONTyped(json, false);
}

export function RequestBodyFromJSONTyped(json: any, ignoreDiscriminator: boolean): RequestBody {
  if ((json === undefined) || (json === null)) {
    return json;
  }
  return {
    'date': !exists(json, 'date') ? undefined : (json['date'] === null ? null : new Date(json['date'])),
  };
}

export function RequestBodyToJSON(value?: RequestBody | null): any {
  if (value === undefined) {
    return undefined;
  }
  if (value === null) {
    return null;
  }
  return {
    'date': value.date === undefined ? undefined : (value.date === null ? null : value.date.toISOString().substr(0,10)),
  };
}

Since the date format of OpenAPI conforms to RFC 3339 full-date, the time zone is not included in the original data string. ex: '2020-04-15'

When converting this original date string with the RequestBodyFromJSONTyped method, specify this string directly in the parameter of new Date, so it will be optimized in the local time zone.

I don't think this behavior is a problem.

But, when converting to JSON with the RequestBodyToJSON method, since it is converted to a string by toISOString, the UTC date is output.
With this, the value changes between conversion and restoration.

I think it should stringify with the date in the timezone of the Date instance.
For example:

`${value.date.getFullYear()}-${(value.date.getMonth() + 1).toString().padStart(2, '0')}-${value.date.getDate().toString().padStart(2, '0')}`

Is this my perception correct? Or am I doing something wrong?

@auto-labeler
Copy link

auto-labeler bot commented Apr 14, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@LeComptoirDesPharmacies
Copy link
Contributor

LeComptoirDesPharmacies commented May 25, 2020

Hi @m-doi2,
Mhhh, I just had the same problem as yours.
I tried your proposal but padStart is a 'ES2017' feature which is not the default compiler option (for better compatibility).

Two other solutions are the following :
Require moment.js and use moment(date).format('YYYY-MM-DD').
Cons : It will ad dependency on generated client which could potentially increase size of projects using this codegen.

Write a "from scratch"method in runtime.js to convert Javascript "Date" type to ISO date string, maybe using this pattern : https://stackoverflow.com/a/25159403

Write a "hacky" method in runtime.js which will use toLocalDateString and transform to align with ISO format :

export function toDateISOString(date: Date): string {
    if (date === undefined) {
        return undefined;
    }
    if (date === null) {
        return null;
    }

    return date.toLocaleDateString(
        'ja-JP',
        {
          year: 'numeric',
          month: '2-digit',
          day: '2-digit'
        }
    ).replace(/\//gi,'-');
}

EDIT : Added some undefined/null check in the method.

Yours faithfully,
LCDP

@mdoi2
Copy link
Author

mdoi2 commented May 26, 2020

Hi, @LeComptoirDesPharmacies

Thank you for your good solution!

I don't have a philosophy on how to implement it at this time, but here are my own thoughts from a general perspective.

Since this is an auto-generated code, I think it's these three points that are important.

  • Performance
  • Code size
  • Does not depend on other packages

That's why I like the method of adding a simple method to runtime. On the other hand, we don't want to use large libraries like moment.js as much as possible.

I hope that a solution to this challenge will be implemented in the generator.

@dfoxg
Copy link

dfoxg commented Feb 25, 2021

Here is the relevant code line: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/typescript-fetch/modelGeneric.mustache#L107

Sadly i´m dont know how fix this line.
In which status is the refactory of the typescript-fetch generator? Maybe this issue will be addressed there.

At the moment i temporary fix the generated code with following lines of code:

from queryParameters['xx'] = (requestParameters.xxas any).toISOString().substr(0,10); to queryParameters['xx'] = timeZoneOffset(requestParameters.xx as any);

function timeZoneOffset(date: Date): string {
	const offset = date.getTimezoneOffset()
	date = new Date(date.getTime() - (offset * 60 * 1000))
	return date.toISOString().split('T')[0]
}

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

2 participants