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

Treat all IEnumerable<T> Properties as arrays #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

brooklynDev
Copy link

Often times, there are classes that have properties of type IEnumerable<T>. The way TypeScripter currently deals with this is by generating a definition for IEnumerable<T> in Typescript. I feel like this isn't really the intent of a user trying to convert a C# class to Typescript. This PR makes it so that any property that implements IEnumerable<T> will get emitted as an array of that type in Typescript.

@brooklynDev
Copy link
Author

After digging through the code some more, I realized that this too was possible without modifying the original source code, rather I was able to inherit from the Scripter class and make the changes there:

    public class CustomScripter : Scripter
    {
        protected override TsType OnResolve(Type type)
        {
            var typeInfo = type.GetTypeInfo();
            if (typeInfo.ImplementedInterfaces.Contains(typeof(IEnumerable)) && typeInfo.IsGenericType && !typeInfo.IsGenericTypeDefinition)
            {
                var elementType = this.Resolve(typeInfo.GetGenericArguments()[0]);
                return new TsArray(elementType, 1);
            }
            else
            {
                return base.OnResolve(type);
            }
        }
    }

I'll leave this open as I still feel that this should be the default behavior, but I would understand your position if you disagree.

@cjlpowers
Copy link
Owner

I agree with you. The intent in nearly all cases will be to emit the array representation. In fact, in practice I run a custom version of this code with a Scripter modification like you made above. So I think this should be the default behavior and not require digging though code and overriding methods. The only concern I have is how that default would be implemented and whether it prevents other "intents".

While I personally wouldn't design a class this way, someone else might.

    public class Group<T> : IEnumerable<T>
    {
        public string Name { get; set; }
    }

    public class Organization
    {
        public List<Group<Employee>> Groups { get; set; }
    }

The PR would generate the following,

declare module TypeScript {
	interface Organization  {
		Groups: TypeScriptGenerator.Employee[][];
	}
}

Might someone want an array of Groups?

My own solution to this problem was the following.

protected override TsType OnResolve(Type type)
{
    if (type.IsGenericType)
    {
        var genericType = type.GetGenericTypeDefinition();
        if (genericType == typeof(List<>) || genericType == typeof(ICollection<>) || genericType == typeof(IEnumerable<>))
        {
            Type keyType = type.GetGenericArguments()[0];
            return Resolve(keyType.MakeArrayType());
        }
    }
    return base.OnResolve(type);
}

The difference is it is more selective about when it emits the array form.

declare module TypeScriptGenerator {
	interface Organization  {
		Groups: TypeScriptGenerator.Group[];
	}
}

Since I cannot say everyone will want the representation in this PR, I think the solution needs to preserve the general purpose type translation logic currently in Scripter as a base implementation for others. One idea that avoids breaking changes would be to introduce a ConfigurableScripter class which supports better configuration and defaults. For example

TypeScripter.Scripter
	.UsingConfig(config) // static method returns new ConfigurableScripter(config)
	.AddTypes(assembly)
        .ToString();

Thoughts?

@brooklynDev
Copy link
Author

brooklynDev commented Apr 6, 2017

I'm still not sold on the idea that treating all IEnumerable<T>s as Arrays should be opt-in, but I hear where you're coming from, that the basic Scripter should be the barebones, with the ability to tweak as needed.

I like your solution of adding a UsingConfig, but I think some thought needs to go into what the config that gets passed into it looks like. Do we keep it strictly to configurations on the Scripter or do we incorporate some other sensible defaults (e.g. see my other PR #14) which don't apply to the Scripter itself but to the TsFormatterinstead.

How about instead of UsingConfig it can be something like WithDefaults or some name that conveys the idea that here's the ability to set up overall defaults to how this will all get generated at the end. (Shooting from the hip here a bit, but I think there needs to be a very transparent and easily recognizable way of setting defaults that many users will likely want).

Separately, I think you did a great job of making everything flexible by making most methods virtual and therefore easily extensible, so with proper documentation (I know, I know, I'm asking for a lot here 😝 ), users will easily be able to tweak it however they see fit.

Once you decide, I'm willing to contribute. Thanks again for a great library! (PS as of now, it's the only one I've found that works with .Net Core)

@cjlpowers
Copy link
Owner

I agree on all points. I am not particularly fond of requiring developers to opt-in to defaults. I would prefer Scripter exhibit the defaults without needing to call UsingConfig or WithDefaults. So I will continue to think about some possible solutions that ideally do not cause a breaking change. Also, my goal would be to cover PR #14 with whatever solution is used.

Regarding extensibility and documentation. This project was born out of a personal need to expose server side models to front end developers writing in TypeScript. So I went pretty heavy on OO to achieve the goal with the mindset that I could override behaviors as necessary for each project. However, that scheme requires a fairly deep understanding of the various components, as you have experienced. I don't think the current scheme is ideally suited for general purpose usage like most folks would expect from a NuGet package. So, where I would like to see this go is to preserve the OO foundation but overlay it with a more sensible API for public consumption that allows people to tweak behaviors more easily. Perhaps it is as simple as providing a set of default implementations of things like Scripter and TsFormatter that expose better configuration capabilities.

Anyway, I am glad you are finding this library useful and would be willing to contribute once we come up with an agreed upon solution.

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.

2 participants