ProductiveRage / Bridge.React

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

Passing sub-set of props to component? #49

Closed kendallb closed 5 years ago

kendallb commented 5 years ago

I am trying to implement the following simple render function in Bridge.React, to see how to convert components to C#:

    // Render the children wrapped in our div
    render() {
        const { verticalClass, firstFocus, ...otherProps } = this.props;
        return <div ref={this.ref} {...otherProps}>{this.props.children}</div>;
    }

What is the normal way to write that in C#? The intent is to remove the props specific to my wrapper from the props and build a new set of 'otherProps' that are passed to the div. Not sure how to wrap my head around how I would do this in C# since it is a strongly typed language?

Also related to that, how do I pass the children for my wrapper component to the div children?

ProductiveRage commented 5 years ago

To pass children to a custom component, you pass them into the base component constructor like this:

public class Wrapper : PureComponent<Wrapper.Props>
{
    public Wrapper(string className, params Union<ReactElement, string>[] children)
        : base(new Props { ClassName = className }, children) { }

    public override ReactElement Render()
    {
        return DOM.Div(new Attributes { ClassName = props.ClassName }, Children);
    }

    public sealed class Props
    {
        public string ClassName { get; set; }
    }
}

..and you would render an instance of it like this.

React.Render(
    new Wrapper("outer", DOM.Div(new Attributes { ClassName = "inner" }, "Hi!")),
    Document.GetElementById("main")
);

Regarding passing a sub-set of props, I'm not so sure. I might have to understand more about what you're trying to do in order to answer the question - wouldn't it be easier in most cases to pass only the props that you want to the "wrapper" class and to specify the props for children on the children elements themselves?

kendallb commented 5 years ago

Thanks for the feedback. The children part makes sense now.

As for the props, sometimes it’s nice to make a wrapper components for stuff like a div element such that your don’t need to care about what props are passed in so you can add say a custom set of classes and pass down all the other props. So then the user if the element can add things like the id, name, style or whatever and it passes those on for it. If we have to declare every prop for a component that needs to be passed on it makes it a lot harder to build such a simple wrapper.

Another good example would be a higher order component wrapper such that you can wrap something and pass on all the props except what you need to the wrapped one. How do you build higher order components like that in C#?

ProductiveRage commented 5 years ago

Higher order components might be written using generic component classes for the wrappers.

I did wonder if you could use a similar approach when passing the props into the wrapper - eg. pass it a name string and a class string and then a TInnerProps instance that would be used to instantiate the child components.. but if you wanted to do something like that then I'm still struggling to understand why you wouldn't let the wrapper component render the outer div (or whatever) using whatever props are passed in and then specify the children components via the children constructor arguments.

I think that the best way to answer the question is to look at a concrete example. And it may be that different types of HOC are better represented in C# in different ways. I'm going to take an example from Understanding React Higher-Order Components by Example, which talks about components that need a saving mechanism that a HOC would provide. To do that, I'd create a class that wraps a generic props type and defines the saving methods:

public sealed class WithStorage<TInnerProps>
{
    private readonly Action<string, object> _save;
    private readonly Func<string, object> _load;
    private readonly Action<string> _remove;
    public WithStorage(Action<string, object> save, Func<string, object> load, Action<string> remove, TInnerProps inner)
    {
        _save = save;
        _load = load;
        _remove = remove;
        Inner = inner;
    }
    public void Save(string key, string data) => _save(key, data);
    public object Load(string key) => _load(key);
    public void Remove(string key) => _remove(key);
    public TInnerProps Inner { get; }
}

Then I'd have a component that has its own Props class but that is derived from a Component that takes a "WithStorage" wrapper around that props class -

public sealed class StorageEnabledComponent : PureComponent<WithStorage<StorageEnabledComponent.Props>>
{
    public StorageEnabledComponent(WithStorage<Props> props) : base(props) { }

    public override ReactElement Render()
    {
        return DOM.Div(new Attributes { ClassName = props.Inner.ClassName },
            // TODO: Real code goes here that makes use of save, load, remove, etc..
            // eg. DOM.Button(new ButtonAttributes { OnClick = props.Save("xyz", 123) }, "Save")
            "Hi!"

        );
    }

    public sealed class Props
    {
        public Props(string className)
        {
            ClassName = className;
        }
        public string ClassName { get; }
    }
}

Then the HOC is defined as a generic component with type param TInnerProps that takes its any props for itself in its constructor and then the props for the wrapped component class and then you also have to tell that HOC how the wrapped component should be instantiated -

public sealed class LocalStorageProvidingComponent<TInnerProps> : PureComponent<LocalStorageProvidingComponent<TInnerProps>.Props>
{
    public delegate ReactElement WrappedFunctionRenderer(WithStorage<TInnerProps> props);
    public LocalStorageProvidingComponent(string className, TInnerProps innerProps, WrappedFunctionRenderer innerRenderer)
        : base(new Props(className, innerProps, innerRenderer)) { }

    public override ReactElement Render()
    {
        return DOM.Div(new Attributes { ClassName = props.ClassName },
            props.InnerRenderer(new WithStorage<TInnerProps>(Save, Load, Remove, props.Inner))
        );
    }

    private void Save(string key, object data) => Window.LocalStorage.SetItem(key, data);
    private object Load(string key) => Window.LocalStorage.GetItem(key);
    private void Remove(string key) => Window.LocalStorage.RemoveItem(key);

    public sealed class Props
    {
        public Props(string className, TInnerProps inner, WrappedFunctionRenderer innerRenderer)
        {
            ClassName = className;
            Inner = inner;
        }
        public string ClassName{ get; }
        public TInnerProps Inner { get; }
        public Func<WithStorage<TInnerProps>, ReactElement> InnerRenderer { get; }
    }
}

This wrapper class creates the props for the inner class by constructing a "WithStorage" instance that combines its own Save, Load and Remove methods with the TInnerProps instances given to the wrapper.

The wrapper can have its own props (just a className in this example) and the inner component can have its own custom props (also just a className here). Different wrappers can implement Save, Load and Remove differently without the inner class having to be altered.

The disadvantages are that the inner component's props have to be accessed via an "Inner" property on the class and the fact that this might well be more code than you would expect to write coming from JavaScript (but the trade-off on that front is that the code is more verifiable so that it's harder for mistakes to creep in or for components to be initialised with invalid or unsupported props).

If you were looking to write a component that injects a property into its children's props (like some toggle show / hide list examples that exist for HOCs) then you might need a slightly different approach. How you model these sorts of things in C# all depends upon what you're trying to make vary from one usage of the HOC to another.

kendallb commented 5 years ago

Ok I think I can see how a HOC could be written using generics, but the lack of the spread operator means writing some code is going to be a lot more verbose:

https://basarat.gitbooks.io/typescript/docs/spread-operator.html

So here is one of the components we use in normal Javascript (not Typescript - I am in the process of deciding if we should go Typescript or C#/Bridge) that makes good use for the spread operator as well as destructuring. It's a simple component that wraps an anchor tag and adds in our custom class (that makes it look like a button). Rather than typing that class in all over the place, I have helper components like this for a, button, div and input submit elements to make it a littler simpler and clearer when we are using these (and later if we decide to change the class name, it's easy).

So how would we implement something like this with Bridge.net and React?

function CssAnchorButton(props) {
    const {className, ...otherProps} = props;
    return <a className={`snap-css-button ${className}`} {...otherProps}/>;
}

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

Been reading your great article series from 2016 about Bridge and React. I am wondering if you have a git project somewhere with the entire source code I could look over more closely?

https://www.productiverage.com/writing-react-apps-using-bridgenet-the-dan-way-from-first-principles

Also something I noticed in the way you write SetState seems off to me. According to the react docs the set state calls are always async, so they do not happen immediately and if you wish to rely on the prior state you have to use the callback version to get access to the current state, which you do not appear to be doing in your examples?

SetState(new State {
    Message = state.Message,       <---- references prior state in the class?
    IsSaveInProgress = true,
    MessageHistory = state.MessageHistory
});

If you are relying on the existing state to create a new state object, things can go sideways. So shouldn't that be written to have a callback to reference the prior state or am I missing something about how React works with Bridge?

Another part that bothers me about state with Bridge/React is that with the lack of the spread operator you cannot spread in existing state, but it also seems there is no way to set partial state values? Or am I missing something there also? It's very common in React to just change one variable in a larger state like so:

updateCouponCode = (e) => {
    this.setState({ couponCode: e.target.value });
};

How do you do partial state updates in React since everything is so strongly typed and the SetState function expects to get a complete state instance every time you call it? State is immutable so you need to clone over the prior state and it would make the SetState calls really verbose if we had to always clone over all the other values like you did with Message = state.Message?``

ProductiveRage commented 5 years ago

Let me get back to you on the usage of SetState and the possibility for stale state - I see exactly what you mean but I can't remember why it's not a problem! (Or, if it is a problem then I'll need to think even more about it!)

For doing partial state updates, my preferred method is to use another of my libraries "Bridge.Immutable" (available on NuGet and the source is on GitHub). I'm hoping that record types in C# 8 will make this unnecessary but, until it does, this tries to simulate it. To use it, you create your Props or State classes such that they implement the empty marker interface IAmImmutable and have all of the properties be readonly and set via a constructor - eg. something a bit like this:

public sealed class State : IAmImmutable
{
    public State(string message, bool isSaveInProgress, string[] messageHistory)
    {
        this.CtorSet(_ => _.Message, message);
        this.CtorSet(_ => _.IsSaveInProgress, isSaveInProgress);
        this.CtorSet(_ => _.MessageHistory, messageHistory);
    }

    public string Message { get; }
    public bool IsSaveInProgress { get; }
    public string[] MessageHistory { get; }
}

This allows you to write code that updates a single field - eg.

var s = new State("", false, new string[0]);
s = s.With(_ => _.Message, "a");

This means that your state update code might look something more like:

updateCouponCode: e => this.setState(this.state.With(_ => _.CouponCode, e.target.value))

Again, this code is more verbose but the library includes an analyser and code fix to make this quicker. You only have to write the class definition and the constructor and the analyser will then see that you have constructor arguments for which there are missing corresponding properties and property-setting statements in the constructor.

Animation of 'populate class from constructor' code fix

The Bridge.Immutable aka ProductiveRage.Immutable library does introduce some other constructs, such as Optional and NonNullList because one of the aims of library is to ensure that there are no more implicit nulls (the bane of my existence!) - there are more details in the Readme for that library (and, hopefully, much of this code can go away if C# 8 introduces nullable reference types).

For your inject-extra-class-names HOC example, I'm afraid that I can't immediately see a way that is super clean in C# but Bridge does have a range of ways to drop down a little closer to the JavaScript if you want to. While you can insert plain JavaScript into the middle of your methods if you want to, a slightly lighter approach that would do the job for us here is that Bridge exposes the Object.GetOwnPropertyNames method and it allows reading and writing properties on objects given their name as all Bridge objects have a string-based indexer. This allows us to write the following method:

private static T CloneAndExtendClassName<T>(T props, string classNameToAdd) 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 (props != null)
    {
        foreach (var propertyName in GetOwnPropertyNames(props))
            clone[propertyName] = props[propertyName];
    }
    clone["className"] = (classNameToAdd + " " + clone["className"]).Trim();
    return clone;
}

.. and we could use that to write a "CssAnchorButton" function like this:

private static ReactElement CssAnchorButton(AnchorAttributes props, params Union<ReactElement, string>[] children)
{
    return DOM.A(
        CloneAndExtendClassName(props, "snap-css-button"),
        children
    );
}

I don't think that I do have a full version of the code from those blog posts, I'm afraid.. I'll have a little look to make sure, though.

kendallb commented 5 years ago

On the issue of stale state, the reason as I understand it is that React batches up state changes and tries to run them all together rather than immediately updating the state (and possibly triggering a render) when setState is called. So if in the process of updating the state in response to data coming back over the wire from a REST call, if the code makes multiple calls to setState() they would all end up being processed in one batch, async.

I suspect in 99% of the cases using the existing state in the class is probably going to work, especially if stuff only ever changes in response to user interaction. But the area where things could potentially go wrong is if two bits of code are making changes to the state that rely on other state bits, that could be set somewhere else. Then you can only rely on the callback method and referencing the prior state to build the new state object (which is a big reason I believe why they added the spread syntax to JSX).

One scenario I could imagine would be if there is state changing based on other async tasks, like timers going off to poll for updated data or data coming over web sockets waiting on a push. If they change state, and something else changes state based on user interaction it would be possible to have a situation where the prior state is not the same as the current state at the time you call setState().

So even in your suggested change to updateCoupon, isn't this.state.Width reference the currently stored state in the class? Surely for this to work correctly we need a new version of setState that has the prior state that is passed to it via React? Otherwise we could still end up with the issue of the state being incorrect if you do something with that state (ie: the state updated based on other components currently in the state).

updateCouponCode: e => this.setState(this.state.With(_ => _.CouponCode, e.target.value))

Also the clone and extend class example is a good idea to look at. I will consider that, because at the end of the day the spread syntax is really just syntactic sugar in JSX that when compiled into Javascript does something similar anyway. It's not real Javascript at all.

Finally I did read up all about your Immutable classes and the one thing I was wondering about, is why you need to have the calls to CtorSet? With C#6 (or is it C#7) the constructor is allowed to write to readonly properties directly like so?

    public class Class1
    {
        public Class1(string name, string email)
        {
            Name = name;
            Email = email;
        }
        public string Name { get; }
        public string Email { get; }
    }

    public class Class2
    {
        public void DoSomething()
        {
            var c = new Class1("Kendall", "joe@blogs.com");
        }
    }
ProductiveRage commented 5 years ago

The "CtorSet" calls are used because I don't want any of the properties on these immutable types to be null and so CtorSet checks the value for null and throw an exception if a null is encountered. When I first wrote this code, if you didn't use CtorSet then you would have to say:

if (name == null)
    throw new ArgumentNullException("name");
Name = name;

When Bridge supported C# 6 syntax, that became a little nicer:

if (name == null)
    throw new ArgumentNullException(nameof(name));
Name = name;

.. and when C# 7 came along, and when Bridge supported it, it could become:

Name = name ?? throw new ArgumentNullException(nameof(name));

The analyser in the that library checks whether each IAmImmutable constructor consists solely of CtorSet calls and comments so that it can offer to auto-populate any constructor arguments / properties that are not set in it. I could feasibly change this to support the format shown above. It feels like a nice improvement, it mostly comes down to making time on my side (such a PR would be welcome, of course!).

Now.. your "SetState" query. The Component class has a method overload for SetState that takes a delegate that is given the current state and props as arguments and which you return a new state instance from - these, according to React, should be guaranteed to not be stale (even if the "this.state" value may be stale) - eg.

public sealed class MyComponent : Component<MyComponent.Props, MyComponent.State>
{
    public MyComponent(string className) : base(new Props(className)) { }

    protected override State GetInitialState() => new State("");

    public override ReactElement Render()
    {
        return DOM.Input(new InputAttributes
        {
            ClassName = props.ClassName,
            Value = state.CouponCode,
            OnChange = e => SetState(
                (state, props) => state.With(_ => _.CouponCode, e.CurrentTarget.Value)
            )
        });
    }

    public sealed class Props : IAmImmutable
    {
        public Props(string className)
        {
            this.CtorSet(_ => _.ClassName, className);
        }
        public string ClassName { get; }
    }

    public sealed class State : IAmImmutable
    {
        public State(string couponCode)
        {
            this.CtorSet(_ => _.CouponCode, couponCode);
        }
        public string CouponCode { get; }
    }
}

I guess that you should always use that method overload. And I presume that that is the case with React components written in JavaScript as well!

I hope you don't mind my closing this issue, hopefully we've covered the original point(s) you raised it for. Feel free to add more questions here, though, if you want me to cover anything else!

kendallb commented 5 years ago

Yeah re-reading the parts about the immutable class that makes sense now to ensure they are never null. Feels like noise in the code but does make it safer.

As for setState, yes, I think we should always be using fhe callback version with the prior state so I will use that. Thanks for the feedback! I have another question but I will open another issue for that.