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

make types nullable #312

Closed
wants to merge 2 commits into from
Closed

make types nullable #312

wants to merge 2 commits into from

Conversation

TimHaboldt
Copy link

Hey.
Was using your typescript compiler for a while and noticed there is no support for the nullable flag. I tried to implement a solution. I hope it satisfies you enough to make it into the main repository. Would be pleased to use this feature in future.
Ps. I implemented a solution that works for me. I cannot promise every possible nullable flag will work with those changed. The most important ones should be covered.
Supported are all basic types and unnamed enums. Named enums, objects and Interfaces shouldn`t be covered.

Regards

@bcherny
Copy link
Owner

bcherny commented Jul 4, 2020

Hey there, thanks for the contribution! I don't see nullable in the JSON-Schema spec -- mind linking to it in your summary?

@TimHaboldt
Copy link
Author

Hey there, thanks for the contribution! I don't see nullable in the JSON-Schema spec -- mind linking to it in your summary?

I looked into it. It was a fault on my part.
It is a specification of the open API Schema (https://swagger.io/specification/) which we are currently using.
Therefore it opens the question if you would like to support the open API Schema too.
I would realize an open API flag you can set inside the options that would allow the open API Schema support.

@zmeyc
Copy link

zmeyc commented Sep 23, 2020

@bcherny We also encountered this issue with nullable types. What do you think about supporting OpenAPI, are any changes needed for this PR or is it out of scope at all?

@bcherny
Copy link
Owner

bcherny commented Dec 19, 2020

Let's support this, but I'd suggest a slightly different implementation: instead of handling isNullable in the parser, generator, and IR, can you instead add a normalizer step that converts isNullable annotations to unions with null (ie. add it to the type array, and make type an array if it wasn't already)?

That way downstream code doesn't need to know anything about isNullable, and it's already handled by existing code that handles unions.

// normalizer.ts
rules.set('OpenAPI: Convert isNullable to union with null', schema => {
  // ...
}

This PR also needs a unit test for your new normalizer rule, as well as an e2e test.

Copy link
Owner

@bcherny bcherny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Let's take one more pass at this.

@zone117x
Copy link

Looks like nullable is also now supported in redoc@2.x. Would be great to have it supported here.
@bcherny I haven't looked into the code for this lib -- do you think implementing the changes you last suggested would be easy for a new contributor?

@TimHaboldt TimHaboldt requested a review from bcherny July 29, 2021 15:27
@TimHaboldt
Copy link
Author

@zone117x I appreciate if someone else could take over. My workload does not allow me to work at anything else right now.

@zone117x
Copy link

I might be able to work on it if @bcherny is available for reviewing the PR and giving guidance where needed (for example how the tests should be structured, etc)

@bcherny
Copy link
Owner

bcherny commented May 22, 2022

Superseded by #411

@bcherny bcherny closed this May 22, 2022
@jackfrankland jackfrankland mentioned this pull request Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants