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

TypeScript types generated as optional but should be required #3244

Closed
nonameolsson opened this issue Sep 1, 2023 · 10 comments · Fixed by #3318
Closed

TypeScript types generated as optional but should be required #3244

nonameolsson opened this issue Sep 1, 2023 · 10 comments · Fixed by #3318
Assignees
Labels
enhancement New feature or request TypeScript Pull requests that update Javascript code WIP
Milestone

Comments

@nonameolsson
Copy link

nonameolsson commented Sep 1, 2023

First of all, I want to thank you for building a really great tool that wilI make development easier 👍

I am using Kiota to generate TypeScript code. It is really easy to use, but I am getting problems with the generated types.

OpenAPI schema

schemas:
  Station:
    type: object
    required:
      - name
      - id

OpenAPI schema with nullable

  schemas:
    Station:
      type: object
      required:
        - name
        - id
      properties:
        id:
          nullable: false
          type: integer
          format: int64
          example: 17
        name:
          nullable: false
          type: string
          example: Trelleborg

Swagger
image

Kiota generated types

export interface Station extends AdditionalDataHolder, Parsable {
    /**
     * Stores additional data not described in the OpenAPI description found when deserializing. Can be used for serialization as well.
     */
    additionalData?: Record<string, unknown>;
    /**
     * The id property
     */
    id?: number | undefined;
    /**
     * The name property
     */
    name?: string | undefined;
}

Notice that both id and name are defined as: name?: string | undefined.
image

Questions

  1. Why are they generated as optional when they are, in OpenAPI, defined as required and non-nullable?
  2. Why is generated both with ? and undefined

Thank you so much for an awesome tool! I do appreciate all feedback on this, so that I an progress.

Additional references

@baywet baywet self-assigned this Sep 1, 2023
@baywet baywet added question TypeScript Pull requests that update Javascript code labels Sep 1, 2023
@baywet baywet added this to the Backlog milestone Sep 1, 2023
@baywet
Copy link
Member

baywet commented Sep 1, 2023

Hi @nonameolsson
Thanks for your interest in kiota and for reaching out.

  1. This is a design decision, lots of APIs allow for field selection via query parameters, or partial representations during updates and since the models can be used both as response and request body, having nullable properties makes them more reusable.
  2. You are correct, this is redundant, on properties we could loose the undefined, we need to keep it on setters since they don't support the ? notation.

More generally we should implement some kind of generated signal to avoid triggering linters for generated code, if this is available for eslint/ts parser. In the meantime we could fix that ad-hoc redundancy, would you be willing to submit a pull request for that?

The code that emits the property is located here

writer.WriteLine($"{conventions.GetAccessModifier(codeElement.Access)} {codeElement.NamePrefix}{codeElement.Name.ToFirstCharacterLowerCase()}{(codeElement.Type.IsNullable ? "?" : string.Empty)}: {returnType}{(isFlagEnum ? "[]" : string.Empty)}{(codeElement.Type.IsNullable ? " | undefined" : string.Empty)};");

@baywet
Copy link
Member

baywet commented Sep 1, 2023

As per the generated annotation, this can be done through comments

/* eslint-disable */
/* generated by ... */

...

/* eslint-enable */

This would need to be implemented here (and in enum and in interface and in function)

_codeUsingWriter.WriteCodeElement(codeElement.Usings, parentNamespace, writer);

As well as the closing equivalent

@microsoft-github-policy-service
Copy link
Contributor

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

@baywet baywet added enhancement New feature or request and removed question Status: No Recent Activity labels Sep 5, 2023
@baywet baywet modified the milestones: Backlog, Kiota v1.7 Sep 5, 2023
@baywet
Copy link
Member

baywet commented Sep 5, 2023

Assigning to myself and moving to 1.7 since @nonameolsson doesn't seem to want to submit a pull request.

@github-project-automation github-project-automation bot moved this to Todo in Kiota Sep 8, 2023
@baywet baywet moved this from Todo to In Progress in Kiota Sep 18, 2023
@baywet
Copy link
Member

baywet commented Sep 18, 2023

Authored #3318 to address the concerns above.

@bkoelman
Copy link

@nonameolsson There's a follow-up issue at #3911, please upvote if you're still interested.

@naeimsf
Copy link

naeimsf commented Feb 7, 2024

Hi @nonameolsson Thanks for your interest in kiota and for reaching out.

  1. This is a design decision, lots of APIs allow for field selection via query parameters, or partial representations during updates and since the models can be used both as response and request body, having nullable properties makes them more reusable.
  2. You are correct, this is redundant, on properties we could loose the undefined, we need to keep it on setters since they don't support the ? notation.

More generally we should implement some kind of generated signal to avoid triggering linters for generated code, if this is available for eslint/ts parser. In the meantime we could fix that ad-hoc redundancy, would you be willing to submit a pull request for that?

The code that emits the property is located here

writer.WriteLine($"{conventions.GetAccessModifier(codeElement.Access)} {codeElement.NamePrefix}{codeElement.Name.ToFirstCharacterLowerCase()}{(codeElement.Type.IsNullable ? "?" : string.Empty)}: {returnType}{(isFlagEnum ? "[]" : string.Empty)}{(codeElement.Type.IsNullable ? " | undefined" : string.Empty)};");

Hi @baywet, thanks for the reply. Anyway to keep some fields required ? (.e.g. Id in this case )

@baywet
Copy link
Member

baywet commented Feb 7, 2024

Not today no

@naeimsf
Copy link

naeimsf commented Feb 7, 2024

anything in the roadmap?

@baywet
Copy link
Member

baywet commented Feb 7, 2024

yes #3911

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request TypeScript Pull requests that update Javascript code WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants