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

ValueTuple support with TypeFunctions.ToTypeScriptType #63

Open
Prinsn opened this issue Aug 25, 2022 · 12 comments
Open

ValueTuple support with TypeFunctions.ToTypeScriptType #63

Prinsn opened this issue Aug 25, 2022 · 12 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Prinsn
Copy link

Prinsn commented Aug 25, 2022

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
Copy link
Author

Prinsn commented Aug 25, 2022

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
Copy link
Contributor

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
Copy link
Author

Prinsn commented Aug 25, 2022

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
Copy link
Contributor

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 NeVeSpl added the enhancement New feature or request label Aug 25, 2022
@NeVeSpl
Copy link
Owner

NeVeSpl commented Aug 25, 2022

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
Copy link
Author

Prinsn commented Aug 25, 2022

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
Copy link
Contributor

gregveres commented Aug 25, 2022 via email

@NeVeSpl
Copy link
Owner

NeVeSpl commented Aug 27, 2022

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.

@NeVeSpl NeVeSpl added help wanted Extra attention is needed good first issue Good for newcomers labels Aug 27, 2022
@gregveres
Copy link
Contributor

@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
Copy link
Owner

NeVeSpl commented Aug 27, 2022

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

@Prinsn
Copy link
Author

Prinsn commented Aug 27, 2022 via email

@NeVeSpl
Copy link
Owner

NeVeSpl commented Aug 27, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants