NeVeSpl / NTypewriter

File/code generator using Scriban text templates populated with C# code metadata from Roslyn API.
https://nevespl.github.io/NTypewriter/
MIT License
126 stars 25 forks source link

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

Closed RudeySH closed 3 years ago

RudeySH commented 3 years ago

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

https://github.com/NeVeSpl/NTypewriter/blob/018ead1c6d6abd87f4125972919e905483d544a7/NTypewriter.CodeModel.Functions/TypeFunctions.ToTypeScriptType.cs#L25

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/NeVeSpl/NTypewriter/blob/018ead1c6d6abd87f4125972919e905483d544a7/NTypewriter.CodeModel.Functions/TypeFunctions.ToTypeScriptType.cs#L31 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 commented 3 years ago

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 commented 3 years ago

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

RudeySH commented 3 years ago

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.