Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

react-component needs to be in array #7

Open
andrewdeandrade opened this issue Dec 13, 2014 · 14 comments
Open

react-component needs to be in array #7

andrewdeandrade opened this issue Dec 13, 2014 · 14 comments

Comments

@andrewdeandrade
Copy link

Currently react-hyperscript requires children react components to be in an array even when there is only one child. Support passing in one child component without having to wrap it in an array.

@mlmorg
Copy link
Owner

mlmorg commented Dec 13, 2014

In my initial PR, I had supported this use-case. In fact, you could pass as many children as you'd like simply as arguments of an element:

h('div',
  h('h1', 'Hello World!'),
  h('ul',
    h('li', 'Stuff')
    h('li', 'More Stuff')));

I ended up removing that, however, and instead implemented a simpler interface where all children (even one) have to be in an array. Check out the discussion here #1 (comment). I'd be happy to support that but it might be useful to do some testing to see what type of de-optimization it would actually cause.

@mlmorg
Copy link
Owner

mlmorg commented Dec 13, 2014

Here's the needed code changes to enable this: https://github.com/mlmorg/react-hyperscript/compare/dev/children-arguments

@andrewdeandrade
Copy link
Author

So I ran a quick and dirty JSPerf, and while this perf test is naive, it shows that all things being equal, that detecting whether or not something is an array or a single element is faster than only accepting arrays as an input. The reason is that when you only accept arrays, you force the user to create an Array unnecessarily, which is a more expensive operation than branching dependent on the argument being an array or not:

http://jsperf.com/detect-array-vs-wrap-arg-in-array

With this in mind, I would be leery of this benchmark in isolation. Other factors that will matter are:

  • the relative proportion of calls where something is a single element. For example, if 90% of the time, the object is an array, you want to optimize for that common case and not worry about the exception where something contains a single element. Given how common it is to nest a single child element, I'm not sure which case is more prevalent.
  • Requiring arrays to be the input results in the user having to create unnecessary garbage (arrays), whereas the different type of input for the children argument results in always having to branch. Having variable types for the children argument will hot function optimizations since the types passed into the function change (array or single object). However I'm not sure it's even optimizable anyways since it's also possible for there to be null value passed for the children when calling h().

Given my experience with browser performance, I would prefer always having to type check and branch accordingly over always having to create an array that will need to be GCed.

@Raynos your thoughts?

@lxe
Copy link

lxe commented Dec 18, 2014

the relative proportion of calls where something is a single element. For example, if 90% of the time, the object is an array, you want to optimize for that common case and not worry about the exception where something contains a single element. Given how common it is to nest a single child element, I'm not sure which case is more prevalent.

That's a common optimization technique. It looks ugly in reality, but it works:

https://github.com/joyent/node/blob/master/lib/events.js#L100-L120

@Raynos
Copy link

Raynos commented Dec 19, 2014

@malandrew my thoughts are h should be consistent.

i.e.

h('div', 'some text');
h('div', { attrs }, 'some text');
h('div', { attrs }, [
  h('child one')
  h('child two', { attrs })
]);

You want the minimal amount of overloading.

Attributes are optional because some things dont have attributes.
Children are optional because some things dont have children

If something has children its either a single text node or a list of children.

Allowing h('div', oneChild) or h('div', childOne, childTwo) adds unneeded complexity.

The biggest part of the unneeded complexity is style in consistency, it means your
templates will be written in one of three styles, you really dont need or want that.

Not to mention that actually using the array makes it easier to break up your element
from its children and just reads cleaner.

@andrewdeandrade
Copy link
Author

@Raynos we're in agreement. The API I'm suggesting is:

h('div', {}, child);
h('div', {}, [child, child]);

What I was trying to avoid if having to put an array when there is only one child:

h('div', {}, [child]);

@Raynos
Copy link

Raynos commented Dec 19, 2014

@malandrew I think h('div', {} , [child]) is fine.

At the end of the day children are an array anyway, you have to allocate that array when you pass it to virtual-dom or react.

@mlmorg
Copy link
Owner

mlmorg commented Dec 19, 2014

@Raynos "reads cleaner" is subjective. Some folks think the closing brackets are harder to read.

@mrozbarry
Copy link

I might be a bit of the niave side of this, but is there any huge performance hit by not enforcing an array at all?

h('div', {}, firstChild, secondChild, thirdChild);

You can grab all of these children internally using arguments, and build an array.

My main cause is that I have been using react with coffeescript, and with this syntax, it would read similar to:

h 'ol', {},
  h 'li', {}, 'First Item'
  h 'li', {}, 'Second Item'

Internally, you'd have to do something like:

childArray = Array.prototype.slice.call(arguments, 2);

I think it offers a good compromise. Backwards compatibility would have to be checked;

if typeof(arguments[2]) == 'array'
  childArray = arguments[2];
else
  childArray = Array.prototype.slice.call(arguments, 2);

I don't know if that's a big performance hit, either. Just a thought.

@richarddewit
Copy link

@mrozbarry +1

@fiatjaf
Copy link

fiatjaf commented Dec 25, 2016

@mrozbarry

if typeof(arguments[2]) == 'array'
  childArray = arguments[2];
else
  childArray = Array.prototype.slice.call(arguments, 2);

Is a performance hit. And it even doesn't account for cases in which you wouldn't have the second argument as props.

@PascalPixel
Copy link

I have tried hyperscript before, but found that is easier to use React.createElement() since it doesn't require an array:

import React from 'react'
const o = React.createElement

const App = () =>
  o('div', {},
    o('h1', {}, 'Hello'),
    o(Foo),
    'Ok!',
    o('p', {}, 'How are you?'),
    o(Foo, {},
      o(Bar, { key: 'hi' },
        o('span', {}, 'Heya!')
      )
    )
  )

It does however, constantly require the {} or null as a second argument, which is annoying.
At this point, is it correct to assume that dropping the array would mean a security or performance hit?

Ref reactjs/rfcs#45

@JAForbes
Copy link

I think if it doesn't have a negative impact on performance it'd be good to support this. Other libraries with similar api's (e.g. mithril) support variadic children. I often jump between frameworks for different projects and it'd be nice to be able to re-use templates with minimal effort and surprise.

@JAForbes
Copy link

found that is easier to use React.createElement() since it doesn't require an array:

True, but it doesn't support css selector syntax, so it's not compatible with some pretty compelling work flows like e.g. bss.

If variadics are supported by React.createElement, then that's another reason to support it in react-hyperscript.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants