Approaches to passing child elements React.createElement #53

ethul · 2015-11-20T04:50:22Z

Currently we are invoking React.createElement in two ways:

React we are passing the child elements as spread arguments to React.createElements.
React.DOM we are passing the child elements as an array argument to React.createElements.

From what I understand, in case (2), react considers the children to be dynamic children; i.e., children can be added, removed, shuffled around, etc. So maybe they are passed in to the component that renders them as an array. The recommendation here is that each child have a key property. Compare this to (1), where passing the children as spread arguments indicates that the children are considered to be known by the implementer. This means they do not need a key property.

I think at both places in the code we may want one behaviour or the other. For example, we may want to write child elements and not have to specify a key when we know all the elements up front (written here in JS):

const html = DOM.div(null, DOM.div(null, 'a'), DOM.div(null, 'b'));

But we also might want to pass a dynamic array of children where a key should be provided (written here in JS).

const values = [ 'a', 'b' ]; // might be passed in as a prop to the component
const html = DOM.div(null, values.map(v => DOM.div({key: v}, v)));

To allow for this in (1), perhaps we can provide a createElementArray function. In (2), for DOM elements, we could maybe add element_ or element'' functions that spread the child element array. However, I am open to ideas and thoughts on this.

The text was updated successfully, but these errors were encountered:

paf31 · 2015-11-24T18:26:36Z

I think we should support both for sure, and I think spread arguments might be a good default.

Maybe createElementDynamic would be a better name? It's not really clear to me how to lift that up to the DOM element level though.

ethul · 2015-11-24T20:07:42Z

Agreed. I think spread does make sense as a default. Also, I like the name
createElementDynamic. Good call.

For DOM elements maybe creating a module React.DOM.Dynamic would work?

On Tuesday, 24 November 2015, Phil Freeman [email protected] wrote:

I think we should support both for sure, and I think spread arguments
might be a good default.

Maybe createElementDynamic would be a better name? It's not really clear
to me how to lift that up to the DOM element level though.

—
Reply to this email directly or view it on GitHub
#53 (comment)
.

paul-rouse · 2015-12-11T09:02:30Z

I can see this is under way, but I came across a specific example which may help the discussion. Passing children as an array in React.DOM.mkDOM causes dangerouslySetInnerHTML to fail a check, at least in React 0.14 (this is with an empty array, of course):

Error: Invariant Violation: Can only set one of `children` or `props.dangerouslySetInnerHTML`.

ethul · 2015-12-11T12:28:49Z

@paul-rouse Thanks for the info. For reference, are you seeing the error with something like (in JS):

React.createElement('DIV', {dangerouslySetInnerHTML: {__html: 'a'}}, []);

paul-rouse · 2015-12-11T12:44:52Z

Yes, I do see the error if I do it like that directly in JS.

I can also confirm that patching React.DOM.mkDOM to do the createElement call the other way fixes the problem when using purescript (I was about to submit an issue when I saw saw this discussion:-)

ethul · 2015-12-11T22:47:29Z

Thanks for confirming. I was testing out the above snippet, but didn't see any warnings from react-0.14.3. Can you confirm exactly which react version you're seeing the warning on? Or do you have anything else in your plain JS example that brings about the warning?

I just am trying to reproduce this locally.

paul-rouse · 2015-12-12T08:56:09Z

Interesting! I'm running it in a browser (Firefox, also checked in Chrome), using React 0.14.0 from the cloudflare cdn, because I based the test on the React tutorial. Here is the complete HTML file - it makes no difference whether it is run from a file or via a server. As shown it provokes the error, and removing the empty children array from createElement makes it work.

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title>React Error</title>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/react/0.14.0/react.js"></script>
    <script src="https://cdnjs.cloudflare.com/ajax/libs/react/0.14.0/react-dom.js"></script>
  </head>
  <body>
    <div id="content"></div>
    <script>
        var Test = React.createClass({
          render: function() {
            return (
              React.createElement('div', {dangerouslySetInnerHTML: {__html: 'a'}}, [])
            );
          }
        });
        ReactDOM.render(
          React.createElement(Test, null),
          document.getElementById('content')
        );
    </script>
  </body>
</html>

ethul · 2015-12-12T18:42:06Z

Thanks for the example. I can reproduce the issue. Also, the following on 0.14.3 shows the issue as well.

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <title>React Error</title>
    <script src="https://fb.me/react-0.14.3.js"></script>
    <script src="https://fb.me/react-dom-0.14.3.js"></script>
  </head>
  <body>
    <div id="content"></div>
    <script>
        ReactDOM.render(
          React.createElement('div', {dangerouslySetInnerHTML: {__html: 'a'}}, []),
          document.getElementById('content')
        );
    </script>
  </body>
</html>

With the patch on React.DOM.mkDOM, the spread approach resolves this since the above createElement call would be like the following with an empty array of elements passed-in:

React.createElement.apply(React, ['div',  {dangerouslySetInnerHTML: {__html: 'a'}}])
// basically the same as:
React.createElement('div', {dangerouslySetInnerHTML: {__html: 'a'}})

I think the limitation is that this error would crop up if the user used an element in the Dynamic module and passed the dangerouslySetInnerHTML property. I am not sure if there is a good way to prevent this case, but maybe we don't have to worry about it because we will support both spread and dynamic approaches. So the user can pick the best one for what they want to do.

ethul · 2015-12-12T18:44:25Z

Actually, in the Dynamic module we can test if the array is empty and not pass it in that case. Not sure if we need to. It might be better to just take the passed array as-is, whether empty or not.

paul-rouse · 2015-12-12T18:58:34Z

Perhaps a note in the documentation for the Dynamic version would be sufficient? I think dangerouslySetInnerHTML is the only property for which the children are checked - see line 140 onward of https://github.com/facebook/react/blob/master/src/renderers/dom/shared/ReactDOMComponent.js

ethul · 2015-12-13T14:30:24Z

Maybe documentation is the way to go. There are indeed a number of invariant checks and warnings in react. If we can add a generic-enough note, that might be ideal.

Thanks for digging into this.

ethul closed this as completed in 2a6d564 Jan 24, 2016

hallettj mentioned this issue Apr 7, 2016

Proposal: alternative method for handling static vs dynamically-generated child element lists #74

Open

safareli mentioned this issue Apr 13, 2018

is "Dynamic" variations of createElement needed? #141

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Approaches to passing child elements React.createElement #53

Approaches to passing child elements React.createElement #53

ethul commented Nov 20, 2015

paf31 commented Nov 24, 2015

ethul commented Nov 24, 2015

paul-rouse commented Dec 11, 2015

ethul commented Dec 11, 2015

paul-rouse commented Dec 11, 2015

ethul commented Dec 11, 2015

paul-rouse commented Dec 12, 2015

ethul commented Dec 12, 2015

ethul commented Dec 12, 2015

paul-rouse commented Dec 12, 2015

ethul commented Dec 13, 2015

Approaches to passing child elements React.createElement #53

Approaches to passing child elements React.createElement #53

Comments

ethul commented Nov 20, 2015

paf31 commented Nov 24, 2015

ethul commented Nov 24, 2015

paul-rouse commented Dec 11, 2015

ethul commented Dec 11, 2015

paul-rouse commented Dec 11, 2015

ethul commented Dec 11, 2015

paul-rouse commented Dec 12, 2015

ethul commented Dec 12, 2015

ethul commented Dec 12, 2015

paul-rouse commented Dec 12, 2015

ethul commented Dec 13, 2015