More convienient element construction through overloads

[Zaid-Ajaj]

Hi, I was playing around with this library because I am new to React and I really like this programming model. However, when constructing complex ui elements the code becomes very ugly. You have to put null where you don't use custom attributes. You have to use ToChildComponentArray() when working with IEnumerable<T>. I took your example on the README page and added a IEnumerable<string> Values property to the State object. Rendering these primitive values looked like this:

DOM.Div(null, 
    DOM.UL(null, state.Values.Select(value => 
            DOM.Li(null, value)).ToChildComponentArray()))

After adding some overloads using the [Template] attribute on methods DOM.Div, DOM.UL and DOM.Li it became this:

DOM.Div(
    DOM.UL(state.Values.Select(DOM.Li))
)

Much nicer, isn't it? Well, borrowing fsharps |> (pipe) operator would be really nice to use here. So we define an extension method:

public static U Pipe<T, U>(this T value, Func<T, U> f)
{
    return f(value);
}

And we use it like this:

state.Values
     .Select(DOM.Li)
     .Pipe(DOM.UL)
     .Pipe(DOM.Div)

The overloads enable us use the functions like this which in my opinion is alot more readable and more intuitive. This pull request defines these overloads only for methods DOM.Div, DOM.UL and DOM.Li to show how this is done and adds an example project for the repo. I look forward to hear your feedback!

[ProductiveRage]

This looks really interesting! I'm on holiday for a week or so, though, so I won't be able to look at this immediately in depth - but I certainly will when I get back.

[Zaid-Ajaj]

Added a canonical Todo app as the example project instead of the "MessageEntryForm". Demo here.

[ProductiveRage]

I've started looking at this - and I like it! :) However, I've got a couple of small questions, one minor concern and one potentially thorny issue.

Firstly, how come you decided on an empty object literal for the default "properties" arguments, rather than null? As you pointed out, before your changes it was necessary to specify null for those values but your changes using the Template attribute (which I really like, by the way, it means that a separate overloads may be added "for free" - it will compile directly to the same JavaScript as it would if the "properties" values were explicitly set, there are no additional methods that must be added to the code to support the new overloads) using an empty object literal instead of null. These empty objects will introduce unnecessary work for the garbage collector to deal with (granted, I don't expect this to be a big problem but it probably makes more sense to use null if there are no reasons not to).

Secondly, why did you add the ObjectLiteral attribute to the props and state classes in your TODO example? I couldn't see any reason why that would be necessary (but I've only performed a preliminary review of the code at this point and played around with the TODO demo - again, nice work).

Thirdly, I've been wondering if there's a way to use the Pipe method with a properties reference - eg.

values.Pipe(DOM.Div, new Attributes { ClassName = "test" })

I've had a little fumble around but I don't think that I have anything that I'm happy with yet.

My minor concern is around the fact that any existing code such as

DOM.div(null, "test")

will now be resolved to the method signature that takes a string array, rather than the original method signature that takes a null properties reference and then a single string child. I don't think that that's a problem since React seems to ignore the null child string - which I think is documented behaviour.

My other (and slightly larger) concern is that "toArray()" is a method that exists on the enumerable type that is returned from Bridge's Linq method implementations but that is not guaranteed to be on all types that implement IEnumerable<T>. I think that it should work ok if the Template attributes are changed from

[Template("React.DOM.div({ }, System.Linq.Enumerable.from({0}).toArray())")]

to

[Template("React.DOM.div({ }, {0}.toArray())")]

I'm still trying to work through all of the possible cases where things could possibly go wrong. I think that there's something really positive here, I just want to be sure that it won't cause any problems with bizarre edge cases.

[Zaid-Ajaj]

Thanks for taking the time to review the code! On your first question, it was just a pragmatic solution and a result of my ignorance of how React works. I firstly passed an empty Attributes class to the constructor, observed that Bridge turned it into { } and that React was happy with it, so I decided to use it. But now looking at React (Virtual) DOM Terminology I see that React indeed just uses null so probably it is best to do the same as well.

On your second question. It is indeed not necessary to use the [ObjectLiteral] to get the code working. It will work either way. However, this decreases the amount of the javascript that Bridge generates, no Bridge.define('class', { . . . }). Another speed up you get is when you instantiate a class that has [ObjectLiteral], Bridge will not use Bridge.merge. So unless I need behavior on my classes, I like to keep them simple (on the javascript part) and use them as data transfer objects.

On Pipe, I am not so sure of what you are trying to accomplish with values.Pipe(DOM.Div, new Attributes { ClassName = "test" }). Could you please elaborate?

On DOM.div(null, "test"). Resolving to string[] is not really a problem. It is however a problem when the second argument is a ReactElement, it will not compile because the call is ambiguous. So yes, it is a breaking change and large existing code bases won't be happy with it.

As for your concern on toArray() this too was just a pragmatic solution, it just worked with my use case so I kept using it. But indeed this line [Template("React.DOM.div({ }, System.Linq.Enumerable.from({0}).toArray())")] looks safer as Enumerable.from does a lot of checks for us. We probably should use this as well instead of [Template("React.DOM.div({ }, {0}.toArray())")]

[ProductiveRage]

Fair point about ObjectLiteral. There are some downsides to using it which you need to be careful about - if you have any methods on your class then they will not be present in the JavaScript and you won't get a compile error about it - the method calls will just fail at runtime with a cryptic error. So I would be wary about applying it to classes by default. However, where you have used it in the TODO app it's totally fine.

When I was talking about supporting a second argument for the Pipe method, I meant that it's not currently possible to specify a properties references, so I can't specify a class name on the "UL" element when I used Pipe - eg.

state.Values
     .Select(DOM.Li)
     .Pipe(DOM.UL)

It might be nice if it was possible to do something like

state.Values
     .Select(DOM.Li)
     .Pipe(DOM.UL, new Attributes { ClassName = "my-list" })

This would create a UL with the LI child elements and set the class "my-list" on the UL. This is only an idea that I was thinking about, it's not something critically important.

I'm feeling uneasy at the prospect of breaking existing code. Where I work, we have got quite a lot of prototype code using this library and I don't really want it to break if I can avoid it. We also have people here getting started with using Bridge and React and I would like to avoid introducing any breaking changes that would cause them problems when they're already in the "slow going" early phase of learning any new technology. From a purely selfish point of view, I've got various posts and tutorials on my blog that use this library and I'd rather avoid having to go back through them and find any code that will no longer work with any breaking changes that we introduce!

However, I am very keen to make improvements to the interfaces such as you have suggested. I wonder if we could add marginally fewer method overloads, so that there are fewer (or, ideally, no) breaking changes..

I appreciate your time on this pull request and I am sure that we will be able to get something good to come from it!

[Zaid-Ajaj]

When having methods on classes with [ObjectLiteral], Bridge will throw TranslatorException with message: ObjectLiteral doesn't support methods, Take a look.

Long explaination Back to Pipe, I understand now. Yes it is very easy to accomplish that. Remember that Pipe has the signature U Pipe<T, U>(this T value, Func<T, U> f). When you write this:

state.Values
     .Select(DOM.Li)
     .Pipe(DOM.UL)

The type of the result of state.Values.Select(DOM.Li) will be your T type and that is ofcourse IEnumerable<ReactElement>. So the second argument Func<T, U> becomes Func<IEnumerable<ReactElement>, U> where U is the type you want to end up with, ReactElement in your case because you want to use DOM.UL. So you just pass in a lambda of type Func<IEnumerable<ReactElement>, ReactElement> like this:

state.Values
     .Select(DOM.Li) // each value from string to ReactElement
     .Pipe(listItems => listItems.ToChildComponentArray()) // from IEnumerable to Array
     .Pipe(listItems => DOM.UL(new Attributes { ClassName = "my-list" }, listItems))

Or if that DOM.UL has an overload to support IEnumerable<T> it becomes just:

state.Values
     .Select(DOM.Li)
     .Pipe(listItems => DOM.UL(new Attributes { ClassName = "my-list" }, listItems))

On the breaking changes, I totally agree with you. But I think the solution is not making less overloads, but actually just to write an overload that will be compatible with DOM.Something(null, items). I will look into it shortly

[Zaid-Ajaj]

Goal: we still want to construct the elements using the params keyword but do not want to provide null everytime we don't use custom attributes and we want to be backwards-compatible!

To ensure backwards compatibility we remove these overloads as they are ambiguous when passing null:

[Template("React.DOM.h3(null, {*children})")]
public extern static ReactElement H3(params Any<ReactElement, string>[] children);

[Template("React.DOM.h3(null, {*children})")]
public extern static ReactElement H3(params ReactElement[] children);

[Template("React.DOM.h3(null, {*children})")]
public extern static ReactElement H3(params string[] children);

But we keep these:

[Template("React.DOM.h3(null, System.Linq.Enumerable.from({0}).toArray())")]
public extern static ReactElement H3(IEnumerable<ReactElement> children);

[Template("React.DOM.h3(null, System.Linq.Enumerable.from({0}).toArray())")]
public extern static ReactElement H3(IEnumerable<string> children);

[Template("React.DOM.h3(null, {0})")]
public extern static ReactElement H3(ReactElement child);

[Template("React.DOM.h3(null, {0})")]
public extern static ReactElement H3(string child);

I think those are enough (for now) as they enable single element construction or a list through IEnumerable<T>.

What about constructing that IEnumerable<T> using the params keyword? Well that's easy with a helper function that someone has to define himself:

IEnumerable<T> ListOf<T>(params T[] items) { return items; } 

Now we can use it as follows:

DOM.Div(
    ListOf(
        DOM.H3("some text"),
        DOM.H3(DOM.Span("Span maybe")),
        DOM.Div(DOM.Div(DOV.Div("Divception")))
    )
)

[ProductiveRage]

Your latest proposition sounds really good. I should be able to look at it properly later today.

Regarding my Pipe question, I was just thinking that

state.Values
     .Select(DOM.Li)
     .Pipe(DOM.UL, new Attributes { ClassName = "my-list" })

looked a bit neater than

state.Values
     .Select(DOM.Li)
     .Pipe(listItems => DOM.UL(new Attributes { ClassName = "my-list" }, listItems))

but there's very little difference in it, to be honest - and the code that you showed (the second of the two snippets above) is still very clear and readable.

[Zaid-Ajaj]

That is still possible, provided you add some more overloads :), one for Pipe

public static ReactElement Pipe<T>(this T value, Func<Attributes, T, ReactElement> f, Attributes attrs)
{
    return f(attrs, value);
}

and 2 other overloads for the DOM.Something methods that are actually the same as the ones added above but with one extra Attributes parameter like this:

[Template("React.DOM.ul({0}, {1})")]
public extern static ReactElement UL(Attributes attrs, ReactElement elem);
                                  UL(Attributes attrs, IEnumerable<ReactElement> elems);
                                  UL(Attributes attrs, string content); // optional
                                  UL(Attributes attrs, IEnumerable<string> values) // optional

and this code will work:

state.Values
     .Select(DOM.Li)
     .Pipe(DOM.UL, new Attributes { ClassName = "my-list" })

Where T will be inferred to be IEnumerable<ReactElement> and will resolve to the second overload above

[ProductiveRage]

Yes, you are quite right - that new Pipe method signature does the job just fine! The reason that I was fighting with it a bit was that I was trying to make it work with custom components (classes that are derived from Component, StatelessComponent or PureComponent) - they aren't actually derived from ReactElement, though they have implicit operator methods so that they can be cast to ReactElement. I don't think that it's worth worrying about right now, though (it certainly shouldn't postpone this PR).

I think that there is now only one potentially breaking change that could get introduced now. Previously, if you wanted to create an empty Div (for example) with no attributes values, then you would have to pass a null properties reference and then specify no children - ie.

DOM.Div(null)

This will now fail since there is no way to unambiguously decide which method signature is the best match.

What you would have to do is to change that code to

DOM.Div((Attributes)null)

or to

DOM.Div(new ReactElement[0])

(the second one is probably preferable, since it most accurately describes the intent - that a Div should be created that has no children).

I don't think that these particular edge cases are anything to worry about.

I'm busy again tomorrow evening but I'm hoping to merge this in (with the Div method signatures shown below) on Saturday and then to look at making the same method signatures available on the other element factory methods some time soon.

[Name("div")]
public extern static ReactElement Div(Attributes properties, params Any<ReactElement, string>[] children);

[Template("React.DOM.div(null, System.Linq.Enumerable.from({0}).toArray())")]
public extern static ReactElement Div(IEnumerable<ReactElement> children);

[Template("React.DOM.div(null, System.Linq.Enumerable.from({0}).toArray())")]
public extern static ReactElement Div(IEnumerable<string> children);

[Template("React.DOM.div(null, {0})")]
public extern static ReactElement Div(ReactElement child);

[Template("React.DOM.div(null, {0})")]
public extern static ReactElement Div(string child);

[Template("React.DOM.div({0}, {1})")]
public extern static ReactElement Div(Attributes properties, IEnumerable<ReactElement> children);

[Template("React.DOM.div({0}, {1})")]
public extern static ReactElement Div(Attributes properties, IEnumerable<string> children);

[Template("React.DOM.div({0}, {1})")]
public extern static ReactElement Div(Attributes properties, ReactElement child);

[Template("React.DOM.div({0}, {1})")]
public extern static ReactElement Div(Attributes properties, string child);

On a side note, I think that looking into all this has helped me with another possible bug elsewhere - so it's been doubly valuable! Once this is merged in and I've investigated that issue properly, I'll confirm that this will all help and then document it.

[ProductiveRage]

These changes have been merged and I've expanded the additional method overloads to the other element factory methods in React.DOM - all included in the pushed-to-NuGet 1.7.0 version of the library.

I had a couple of tweaks to make to the TODO sample application that you might be interested in checking out, there were React warnings about an absence of unique keys on dynamic children elements that I've corrected (see https://facebook.github.io/react/docs/multiple-components.html#dynamic-children).

Thanks again for your valuable input with this pull request!

[Zaid-Ajaj]

No problem, I am glad I was able to help!