ProductiveRage / Bridge.React

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

Component with a single ReactElement child throws "Not Valid React Child" #20

Closed badcommandorfilename closed 7 years ago

badcommandorfilename commented 7 years ago

I'm trying to create a re-usable popover component that accepts some arbitrary ReactElements as children.

When I construct my component with a single child param like this:

            var iframe = DOM.IFrame(new IFrameAttributes { Src = url, Style = new ReactStyle { Width = "inherit", Height = "inherit" } });

            var component = new Popover(
                new Popover.Props { Active = true }, //props
                iframe); // type is params Union<ReactElement, string>[] children 

I get the following runtime exception when the component tries to render the children:

UIComponents.js:27355 Uncaught Error: Objects are not valid as a React child (found: object with keys {key, value}). If you meant to render a collection of children, use an array instead or wrap the object using createFragment(object) from the React add-ons. Check the render method of Popover . at invariant (react-dom.js:17985) at traverseAllChildrenImpl (react-dom.js:16688) at traverseAllChildrenImpl (react-dom.js:16637) at traverseAllChildren (react-dom.js:16716) at Object.instantiateChildren (react-dom.js:4345) at ReactDOMComponent._reconcilerInstantiateChildren (react-dom.js:10434) at ReactDOMComponent.mountChildren (react-dom.js:10473) at ReactDOMComponent._createInitialChildren (react-dom.js:6216) at ReactDOMComponent.mountComponent (react-dom.js:6035) at Object.mountComponent (react-dom.js:11598)

Indeed, the "iframe" ReactElement has been unpacked into a json dictionary. This is probably a Bridge.NET edge case.

However, if add a dummy div element to the params list, the params type is preserved properly:

            var div = DOM.Div(new Attributes());
            var iframe = DOM.IFrame(new IFrameAttributes { Src = url, Style = new ReactStyle { Width = "inherit", Height = "inherit" } });

            var component = new Popover(
                new Popover.Props { Active = true }, //props
                iframe,
                div
            );

The workaround of adding a dummy element is fine for my purposes now, but it seems to be a weird case with Bridge.NET type conversion.

As always, thanks for all your fantastic work!

Version Info:

<packages>
  <package id="Bridge" version="15.7.0" targetFramework="net461" />
  <package id="Bridge.Core" version="15.7.0" targetFramework="net461" />
  <package id="Bridge.Html5" version="15.7.0" targetFramework="net461" />
  <package id="Bridge.Min" version="15.7.0" targetFramework="net461" />
  <package id="Bridge.React" version="1.12.6" targetFramework="net461" />
</packages>
ProductiveRage commented 7 years ago

I'm having a little bit of trouble reproducing what you're seeing. Could you include a min repro case based on your Popover component so that I can see what code you're executing within the Render method in order to display the child components?

badcommandorfilename commented 7 years ago

Hi Dan, thanks for the help!

I'll try to put together a reproduction project for you - in the meantime I suspect that it might be something to do with how I'm inserting the Child elements in the Render method:

        public override ReactElement Render()
        {
            var classname = "modal";
            if(state.IsActive)
            {
                classname += " is-active";
            }

            return DOM.Div(new Attributes { Id = props.InnerID, ClassName = classname },
                    DOM.Div(new Attributes
                        {
                            ClassName = "card",
                            Style = new ReactStyle { Width = 350, Height = 480 }
                        },
                        Children.AsEnumerable().Cast<ReactElement>()), //<-- This is possibly non-standard?
                    DOM.A(new AnchorAttributes
                    {
                        ClassName = "close",
                        OnClick = (e) => SetState(new State { IsActive = false })
                    })
                );
        }

I've used "Children.AsEnumerable().Cast()" because the type of "Children" is Union<ReactElement, string>[] so there is a warning if you try to just use them directly in Render:

When calling element factory methods that accept children through a params array (such as Div), the children should not be specified through a single array reference since this bypasses important logic in React about unique keys being present on dynamic children - instead, use one of the alternative method signatures that accepts an IEnumerable of ReactElement or of custom Stateless or Pure Components (see https://facebook.github.io/react/docs/reconciliation.html for why unique keys for dynamic children are important)

Could this be relevant? Thanks!

ProductiveRage commented 7 years ago

I think that there are a few problems.

Firstly, the warning about dynamic children is important but I don't think that it should apply when using the "Children" property of a Component class - otherwise you'll always get it when trying to render Children elements in custom components, as you have seen here!

When this is done, maybe the analyser should apply the same logic to the last argument of custom components (if it's of type "params Union<ReactElement, string>[]") so that it's not too easy to create sets of dynamic children without giving them all unique keys.

Lastly, there is nothing that looks obviously wrong with the format of "Children.AsEnumerable().Cast()" and that would appear to be a separate issue. I will investigate that first in case it is a Bridge issue!

ProductiveRage commented 7 years ago

I've reproduced the problem using this:

public sealed class Popover : PureComponent<object>
{
    public Popover(params Union<ReactElement, string>[] children)
        : base(null, children) { }

    public override ReactElement Render()
    {
        return DOM.Div(null,
            Children.AsEnumerable().Cast<ReactElement>()
        );
    }
}

This works:

React.Render(
    new Popover(
        DOM.Div("Item0"),
        DOM.Div("Item1")
    ),
    Document.GetElementById("main")
);

This doesn't:

React.Render(
    new Popover(
        DOM.Div("Item0")
    ),
    Document.GetElementById("main")
);

(I get the same error as you; "Objects are not valid as a React child").

ProductiveRage commented 7 years ago

I think that there is some sort of Bridge-related oddness around that "Cast<ReactElement>()" call that I need to investigate further (if there is a bug then I'll raise it with the Bridge Team and record it here).

The good news is that it all works correctly is you just "Children" instead of "Children.AsEnumerable().Cast<ReactElement>()". I hope to remove the warning about using the Children property within custom components soon. In the meantime, though, you could put the following line at the top of your component classes that need to use the "Children" property -

#pragma warning disable BridgeReact // Ignore Bridge.React warnings until https://github.com/ProductiveRage/Bridge.React/issues/20 is resolved

and then you won't get the warning. After I've sorted things out properly, you can remove that line again :)

ProductiveRage commented 7 years ago

I got to the bottom of this, it was actually a behaviour in React.

When React.createElement is called with no children then it stores props.children as a null(ish) value, if it's called with multiple children then it's stored as an array but if it's called with a single child then it's stored as a single object. I presumed that one child would be stored the same as multiple and so a non-array object was being accessed as an array, causing confusion. I've changed the bindings to ensure that the "Children" property always returns an array properly.

This fix will be in the next release of the bindings, which should be available early next week.

(I've just remembered that I said that I would change the analyser that was causing the "dynamic children" warning when the "Children" property was accessed - I will also do that and include it in the next release).