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

[REQ][typescript-inversify] Change 'optional' behavior #5543

Closed
siada opened this issue Mar 6, 2020 · 7 comments
Closed

[REQ][typescript-inversify] Change 'optional' behavior #5543

siada opened this issue Mar 6, 2020 · 7 comments

Comments

@siada
Copy link
Contributor

siada commented Mar 6, 2020

Is your feature request related to a problem? Please describe.

When generating models for a .net API, I have to add the [Required] attribute to every model property to avoid the typescript-inversify generator creating view model properties as optional.

The problem with them being optional is typescripts 'undefined by default' approach - you end up with a lot of undefined checks against properties that are never going to be undefined.

The problem this causes is the model validator on the .net side is validating properties as [Required] that are actually allowed to be null - but they're defined as [Required] so typescript-inversify doesn't try to make them undefined

Describe the solution you'd like

Change the default behaviour of declaring a typescript-inversify model property from:

name?: string

to a union type:

name: string | null

@auto-labeler
Copy link

auto-labeler bot commented Mar 6, 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.

@amakhrov
Copy link
Contributor

amakhrov commented Mar 7, 2020

Isn't nullable what you are looking for?

However, in general I don't see a practical difference between undefined and null. Could you please clarify why do you want to handle them differently in your typescript code?

TSLint even has a rule that bans using null in a codebase (and provides some good reasons for that)

@siada
Copy link
Contributor Author

siada commented Mar 7, 2020

You might be right that nullable is what I'm after, but that's not supported in the typescript-inversify generator:

https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/typescript-inversify/modelGeneric.mustache#L8

An example of the benefit's I can see to this change(whether it be nullable or changing the behavior of required) is on a view model, I've defined something such as:

name: string;
jobTitle: string;

It's perfectly acceptable for these fields to be null, however they will never be undefined. The problem I have is that I map from an api model, to this view model for rendering, and the api model generates as:

name?: string;
jobTitle?: string;

Which then forces me to do:

const response: PersonViewModel = {
    name: apiResponse.name || null,
    jobTitle: apiResponse.jobTitle || null
};

Otherwise the linter throws a bitch fit because string | undefined cannot be assigned to string.

The ? from the type disappears if I mark the fields as [Required] in my api, but this then breaks the contract because the api will reject if the properties are null, but I actually expect them on certain conditions to be null, they're only [Required] so that typescript-inversify doesn't generate them as | undefined

@amakhrov
Copy link
Contributor

amakhrov commented Mar 8, 2020

Ok, so what I hear is that typescript-inversify generator lacks support for nullable (unlike some other typescript generators).
Would you want to file a PR fixing that?

@siada
Copy link
Contributor Author

siada commented Mar 8, 2020

I'll submit the PR and see if that fixes my issue

@siada
Copy link
Contributor Author

siada commented Mar 9, 2020

PR Submitted #5568

@macjohnny
Copy link
Member

fixed with #5568

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

3 participants