NeVeSpl / NTypewriter

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

ValueTuple support with TypeFunctions.ToTypeScriptType #63

Open Prinsn opened 2 years ago

Prinsn commented 2 years ago

for the class

public foo {
  public (int, string) easy {get;set}
  public IEnumerable<(int, string)> hard {get;set}
}

which will emit for types using ToTypeScriptType

easy: <number, string>;
hard: <number, string>[];

I know this is kind of a weird case because I don't know that there is a type for ValueTuple to map to, and we got around this by supplying our own type(s)

export interface ValueTuple<TOne, TTwo> {
    item1: TOne,
    item2: TTwo
}

The easy case is solved easy enough with custom script

public static string ToTypeScriptType(IType type, IClass c)
{
  var prefix = type.Name.StartsWith("(") ? "ValueTuple" : "";
  return prefix + TypeFunctions.ToTypeScriptType(type);
 }

But trying to solve it generically for any instance of a value tuple seems to require custom rewrite of ToTypeScriptType to make use of this.

Prinsn commented 2 years ago

Having copied the TypeScriptionFunctions locally, appending to TranslateNameToTypeScriptName

if (type.Name.StartsWith("("))
 return "ValueTuple";

Appears to work in this case, so whatever analysis would be used to determine it exists seems sufficient.

gregveres commented 2 years ago

I don't know if you want to go down this path but here is how I solved things like this.

First, I will say that I already flag the types I want to export with an attribut called "ExportToTypescript", so our solution was already prepared to use attributes.

We ran into a case where we really wanted some strings to be nullable and we are stuck on C# 7.3 for the moment, so we couldn't adopt string? as the type. So we introduced another attribute called [TypescriptType(type= "string | null")].

    [AttributeUsage(AttributeTargets.Property | AttributeTargets.Class)]
    public class TypescriptType: Attribute
    {
        public string Type { get; set; }

        public override string ToString()
        {
            return Type;
        }
    }

Then I use it like this:

        [ExportToTypescript]
        public class CartCheckoutInfo
        {
            [TypescriptType(Type = "string | null")]
            public string Message { get; set; }
            public string PaymentId { get; set; }
        }

And to extract the "string | null",

        public static string ToTypeScriptType(IProperty prop)
        {
            var attr = prop.Attributes.FirstOrDefault(a => a.Name == "TypescriptType") ?? prop.Type.Attributes.FirstOrDefault(a => a.Name == "TypescriptType");
            var attrType = attr?.Arguments.FirstOrDefault()?.Value;
            if (attrType != null) return attrType as string;

            return prop.Type.ToTypeScriptType();
        }

I know this is more manual, but it gives you the ultimate control over the type exported.

Prinsn commented 2 years ago

That does seem like it'll be worth keeping in mind, though the only type that seems to have given us any problems currently is this specific case.

We've not run into any issues with your cited case, however, with Typewriter definitely not emitting | null I was actually surprised to see it pop up in NTypewriter generation since it is more correct.

gregveres commented 2 years ago

The other place where we used it was for a DBId class we have. We return hashed strings for the Id field of a db record, but we have a class that fairly seemlessly translates between the string Id and the numeric Id from the database. We used this attribute to have NTypewriter output a string type for this DBId type.

We haven't run across any other use cases yet, and we wouldn't have needed it if we could move up to a newer version of C# that allows for nullable types.

NeVeSpl commented 2 years ago

I will think about it. I have just released version v0.3.6 which contains a commit that exposes IType.IsTuple, that should help eliminate that ugly string comparison.

Prinsn commented 2 years ago

I'm really not sure what you would see as optimal, but since ValueTuple is a type that serializes just fine, but has no native type, it seems like the most straightforward thing would be a configuration parameter for ValueTupleTypesScriptType that gets utilized pulled there?

I don't know if there are any such similar types you'd have to handle for.

gregveres commented 2 years ago

The other thing that I haven’t seen mentioned is named tuple values.

When tuples first came out we used them like your example does, using item1 etc. But when support came out for named tuples, we converted everything over to them. Providing the names gave the code so much more meaning.

I would expect a serialization that used the names would be very useful, but I haven’t tried to see if .net injests them as expected or not.

On Aug 25, 2022, at 2:48 PM, Prinsn @.***> wrote:

 I'm really not sure what you would see as optimal, but since ValueTuple is a type that serializes just fine, but has no native type, it seems like the most straightforward thing would be a configuration parameter for ValueTupleTypesScriptType that gets utilized pulled there?

I don't know if there are any such similar types you'd have to handle for.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.

NeVeSpl commented 2 years ago

I do not think that we need a parameter for that, ValueTuple can be hard coded, it is the name of type that the compiler uses internally anyway.

@Prinsn you are welcome to prepare PR that adds support for ValueTuples to ToTypeScriptType. Do not forget to write tests.

gregveres commented 2 years ago

@NeVeSpl I am not sure if the comment of "I do not think we need a parameter for that" was aimed at my comment about named tuples. If it was, then I think we are miscommunicating.

Named tuples allows you to write:

(string msg, boolean flag) SomeFunction() {}

This should serialize to

{msg: string, flag: bool} function SomeFunction() {}

instead of

{item1: string, item2: bool} function SomeFunction() {}
NeVeSpl commented 2 years ago

It was not, @Prinsn proposed adding ValueTupleTypesScriptType parameter, and I only commented on his proposal, without making any remarks regarding named tuples,

Prinsn commented 2 years ago

Is it sufficient to your project that it just emit ValueTuple and just not compile in TS?

It works for our purposes because we already "worked around it" with Typewriter, it was just a matter of getting template generation to parity.

I'm not sure I'll get to a PR since if I start down that road I'll want to poke around ToCamelCase because I had to hack around that to get it to parity with Typewriter (in one way that made me question how the hell Typewriter even went about it).

For what it's worth, just adding the IsTuple case appeared to work, just a two line add, though I'm not sure if serializing Tuples would conflict at all in this case, as we aren't using them in any serialization.

On Sat, Aug 27, 2022, 8:27 AM NeVeS @.***> wrote:

I do not think that we need a parameter for that, ValueTuple can be hard coded, it is the name of type that the compiler uses internally anyway.

@Prinsn https://github.com/Prinsn you are welcome to prepare PR that adds support for ValueTuples to ToTypeScriptType. Do not forget to write tests.

— Reply to this email directly, view it on GitHub https://github.com/NeVeSpl/NTypewriter/issues/63#issuecomment-1229183294, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAYR7G6FP7C6GM7XXCUU34DV3ICS3ANCNFSM57TGZRQQ . You are receiving this because you were mentioned.Message ID: @.***>

NeVeSpl commented 2 years ago

Right now it does not generate valid TS for tuples either, so having a type name there would definitely be a step forward. But since it is not a very popular thing, it can stay as it is, your choice.