mlmorg / react-hyperscript

Hyperscript syntax for React.js markup
MIT License
710 stars 45 forks source link

react-component needs to be in array #7

Open andrewdeandrade opened 9 years ago

andrewdeandrade commented 9 years ago

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 commented 9 years ago

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 https://github.com/mlmorg/react-hyperscript/pull/1#discussion_r18723100. 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 commented 9 years ago

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

andrewdeandrade commented 9 years ago

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:

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 commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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

mrozbarry commented 9 years ago

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 commented 8 years ago

@mrozbarry +1

fiatjaf commented 7 years ago

@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 commented 6 years ago

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 https://github.com/reactjs/rfcs/issues/45

JAForbes commented 5 years ago

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 commented 5 years ago

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.