ProductiveRage / Bridge.React

Bindings for Bridge.NET for React - write React applications in C#!
MIT License
74 stars 14 forks source link

Flatten sealed attributes with C# binding to external component? #54

Closed kendallb closed 5 years ago

kendallb commented 5 years ago

Reading through the changes to the DOM attributes in React 16, I think it makes sense for the attributes classes to be sealed:

https://reactjs.org/blog/2017/09/08/dom-attributes-in-react-16.html

In React 16 any unrecognized attributes will be passed through unchanged to the DOM element, not just the data- and aria- ones they supported before. Hence if the class is extended and passed into a lower level component without first being 'cleaned' (ie: using the spread operator to remove the custom props), those props would end up in the DOM elements which is not what you want.

So given that, we don't really have a clean solution for implementing the wrapper I am working on for the React Number Format component. I am thinking maybe a reasonable solution for the C# wrapper which would make it a little different to the Javascript version, would be to make the InputAttributes a property in the props itself? So something like this:

        [ObjectLiteral]
        public class Props : ReactDomElementAttributes<HTMLInputElement>
        {
            public InputAttributes Attributes { private get; set; }
            public string MyCustomProp { private get; set; }
        }

So to use it, you would create a separate class for the real input attributes and fill in the rest of the attributes for the class itself separately. The problem then is that before it is passed to the real implementation, we would need to pass it through some code to flatten the structure. But I am not sure how we can link this into the constructor call? I currently have this for the constructor:

        [Template("React.createElement(NumberFormat, Bridge.React.fixAttr({props}))")]
        public NumberFormat(
            Props props)
        {
        }

But rather than calling Bridge.React.fixAttr() I could pass it through my own function to 'flatten' the attributes. The question then is, is it possible to output some custom Javascript for the flattening, or can I write my flattening code as a helper in C# and somehow have the wrapper call that?

ProductiveRage commented 5 years ago

Something like the "CloneAndExtendClassName" method in issue #49?

kendallb commented 5 years ago

I ended up with this, but it is not fully tested yet. I think it will do the job.

        /// <summary>
        /// Clone a properties element and flatten any sub-properties into the same structure
        /// </summary>
        /// <typeparam name="T">Type of the properties to clone</typeparam>
        /// <param name="props">Properties to clone</param>
        /// <param name="flattenName">Name of the property to flatten</param>
        /// <returns>Cloned properties</returns>
        public static T CloneAndFlattenProps<T>(
            T props,
            string flattenName)
            where T : new()
        {
            // As DomElementsAttributes is an [ObjectLiteral], there is no Bridge state-tracking funny business to
            // worry about when cloning it - it's just a simple JavaScript object
            var clone = new T();
            if (props != null) {
                foreach (var propertyName in GetOwnPropertyNames(props)) {
                    if (propertyName == flattenName) {
                        var toFlatten = props[propertyName];
                        foreach (var flattenedPropertyName in GetOwnPropertyNames(toFlatten)) {
                            clone[flattenedPropertyName] = toFlatten[flattenedPropertyName];
                        }
                    } else {
                        clone[propertyName] = props[propertyName];
                    }
                }
            }
            return clone;
        }
ProductiveRage commented 5 years ago

Did you get a chance to test this? The comment in the code says "As DomElementsAttributes is an [ObjectLiteral]" but type param T is not constrained to being a DomElementsAttributes. I guess that the reason for this is that T is not necessarily a DomElementsAttributes since it's a new kind of props class that has one property that is derived from DomElementsAttributes and then one another property that isn't. You could make this implement an interface that has these two properties but that might feel like jumping through hoops just to appease the type system that is supposed to be helping us. Maybe an alternative would be to write a clone method that takes the DomElementsAttributes props (instead of the NumberFormat.Props class) and the property to set and what value it should get. Like this:

public static class ReactComponentHelpers
{f
    public static T CloneAndSet<T>(T source, string overwritePropertyName, object overwritePropertyValue)  where T : DomElementsAttributes, new()
    {
        // As DomElementsAttributes is an [ObjectLiteral], there is no Bridge state-tracking funny
        // business to worry about when cloning it - it's just a simple JavaScript object
        var clone = new T();
        if (source != null)
        {
            foreach (var propertyName in GetOwnPropertyNames(source))
                clone[propertyName] = source[propertyName];
        }
        clone[overwritePropertyName] = insertPropertyValue;
        return clone;    
    }
}

.. then you would call that from the constructor (and then call fixAttr on the result) -

[Template("React.createElement(NumberFormat, Bridge.React.fixAttr(MyNamespace.NumberFormat.FlattenProps({props})))")]
kendallb commented 5 years ago

I have checked the generated code but not had a chance to test it just yet. The way I wrote mine, I pass in a new property structure to override the original one as it clones it, so I can update multiple props at the same time.

ProductiveRage commented 5 years ago

I'll close this for now, then. Feel free to reopen if you need more from me.

(Do you think that the NumberFormat.Props class should be sealed?)

kendallb commented 5 years ago

Yes, I did make NumberFormat props sealed in my implementation for the reasons we discussed.