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

Ability to configure ToTypeScriptType to handle nullable types differently. #4

Closed
RudeySH opened this issue Mar 9, 2021 · 6 comments · Fixed by #7
Closed

Ability to configure ToTypeScriptType to handle nullable types differently. #4

RudeySH opened this issue Mar 9, 2021 · 6 comments · Fixed by #7
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@RudeySH
Copy link
Contributor

RudeySH commented Mar 9, 2021

Currently, NTypewriter appends " | null" to nullable types:

I would like to be able to use any of following TypeScript notations in my templates:

myProperty: string | null;
myProperty: string | undefined;
myProperty: string | null | undefined;
myProperty?: string;

I could attempt to write my own implementation of ToTypeScriptType, but I'd rather not copy-paste 100+ lines to change a single detail. I'd like to help out if possible, but I'm not sure where to start - I'm not sure what the best approach is here. Is there already an example of something similar to this in the code base, e.g. a configuration option that affects the behavior of a static function such as ToTypeScriptType?

@NeVeSpl
Copy link
Owner

NeVeSpl commented Mar 9, 2021

There is no global configuration for custom functions available, but in NTypewriter (contrary to Typewriter) custom functions can have parameters, thus I would just add an additional optional parameter to this method to affect its behaviour.

You are welcome to prepare a pull request, just do not forget to include tests for this change.

@RudeySH
Copy link
Contributor Author

RudeySH commented Mar 9, 2021

I would like to add a function similar to ToTypeScriptType that does not append anything for nullables. Such a function would be much more flexible, you can now easily append a postfix yourself, like so:

{{ if property.Type.IsNullable }} | undefined{{ end }}

Full example:

{{ property.Name | String.ToCamelCase }}: {{ property.Type | Type.ToTypeScriptType }}{{ if property.Type.IsNullable }} | undefined{{ end }};

This would cover all cases, but requires more code in the template (not a bad thing in my opinion).

What are your thoughts on this? If you agree, what should this function be named so that the distinction between ToTypeScriptType and the new function is obvious? Ideally the behavior of ToTypeScriptType could be changed to avoid polluting the interface, but that would be a breaking change.

@NeVeSpl NeVeSpl added the enhancement New feature or request label Mar 9, 2021
@NeVeSpl
Copy link
Owner

NeVeSpl commented Mar 10, 2021

At the current stage, breaking changes are not a problem. But in that case, I would leave the behaviour of ToTypeScriptType as it is, because | null is the most accurate equivalent of nullable type.

The function that you described already exists, only needs a few tweaks: add null checking, documentation, make it public

private static string ToTypeScriptTypePhase2(IType type)

But the name is awful and it shows that I did not have an idea how to name it. Something better is needed, that will underline distinction. And most obvious choices seem too long :
ToTypeScriptTypeNotNullable
ToTypeScriptTypeRaw
ToTypeScriptTypeExtant
and more ideas on how to replace the phrase not null:
https://stackoverflow.com/questions/14273820/is-there-a-single-word-meaning-not-null

@RudeySH
Copy link
Contributor Author

RudeySH commented Mar 10, 2021

I know you just said you want to leave the behavior of ToTypeScriptType as-is, but I'll make the following suggestions anyway:

How about renaming the existing function to ToTypeScriptUnionType and use ToTypeScriptType for the non-nullable variant?

Or, perhaps even better, an optional boolean argument named "nullable". It could be true by default if that's the default behavior you'd want.

@NeVeSpl
Copy link
Owner

NeVeSpl commented Mar 10, 2021

I would go with the proposed nullable parameter, it seems most obvious from a user perspective what it does.

@RudeySH
Copy link
Contributor Author

RudeySH commented Mar 11, 2021

After doing some work on this, I realized that having only an option to exclude the nullable postfix isn't going to be sufficient for my use case. Arrays and generics make things a little more complicated. The function currently produces the following:

myProperty: int | null[]
myProperty: MyGeneric<int | null>

In these cases, I'd like to be able to configure NTypewriter to use | undefined instead.

By the way, int | null[] is a bug. This is a type that can either be an int, or an array containing nulls. This does not represent an array that contains nullable ints.. It should be (int | null)[] or Array<int | null> instead. I will create a pull request soon that fixes this problem.

NeVeSpl added a commit that referenced this issue Mar 13, 2021
implement configurable nullable type, resolves #4
@NeVeSpl NeVeSpl added this to the 0.0.6 milestone Apr 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants