ProductiveRage / Bridge.React

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

Why do DOM element attributes have private getters? #57

Closed kendallb closed 5 years ago

kendallb commented 5 years ago

What is the reason that the DOM attributes classes all have private getters for all the fields? If I am building a wrapper class around say a button element, and want to extend the class name, I am not sure how I can implement this if I cannot see what the original class name passed into the component is since the getter is private? I am trying to implement something like this:

function CssSubmitButton(props) {
    const {className, ...otherProps} = props;
    return <button type="submit" className={`snap-css-button ${className}`} {...otherProps}/>;
}

and the C# version would look something like the following. Without the spread syntax it makes things a little more complicated, but given that the props passed to be will be in a new instance for the render call, it seems to me I should be able to modify it. But with the way the library is at the moment, I cannot do that?

        public override ReactElement Render()
        {
            props.ClassName = $"snap-css-button {props.ClassName}";
            return DOM.Button(props);
        }
kendallb commented 5 years ago

Bah, we covered this already in another ticket. Now sure how to delete issues?

kendallb commented 5 years ago

Although it does seem to me that it would be simpler to be able to read the value and assign a new value, that having to clone the whole thing and update the class name as you suggested in an earlier thread?

kendallb commented 5 years ago

I just realized that I can access the internals of the props using the array indexer and name, like this:

props["className"]

Given that we can easily read the values using an array indexer (as shown in your example for how to extend the classname), it seems to me that making them have a private getter is a bit pointless since I can read the value anyway? Wouldn't it be better to be able to read it in a type safe manner? Or am I missing something?

ProductiveRage commented 5 years ago

If you're happy that this was addressed elsewhere then I'll close it.

You can access data via the string-property-name-based indexer that is on every Bridge object but I think that you should avoid it, really. To be honest, I think that it was a mistake of the Bridge Team to expose it -I imagine that they did it to try to make JavaScript-interacting code easier but it comes at the cost of type safety. For example, you can do this:

public class Program
{
    public static void Main()
    {
        var x = new MyType("test");
        Console.WriteLine(x.Name);
        x["Name"] = "OVERWRITTEN!";
        Console.WriteLine(x.Name);
    }
}

public class MyType
{
    public MyType(string name) => Name = name;
    public string Name { get; }
}

(See here: https://deck.net/04751c9a258e3342f0220689d3bfb87c)

I think that allowing that so easily through C# code is a terrible idea and that it should be much more clearly a "code smell". It's one thing allowing you to drop into JavaScript to do some magic that skirts around the type system but it's another to allow it in a way that looks so innocuous on the surface.

kendallb commented 5 years ago

Yes I agree access via the array indexer is not clean at all, but if that is removed then the attributes have to be non -private otherwise there is no way to be able to read existing ones like className and extend them? I don’t really see any valid reason for them to be private? They are not like that in any of the retyped libraries.

kendallb commented 5 years ago

Can we re-open this one, because I honestly think it is the incorrect approach to have all the properties mapped with private getters, as then we have no way to use those values for building wrapper components.

I would be happy to make the code changes as a PR if you like.