Hyperapp: Change component signature from (props, children) to (props) and props.children

[frenzzy]

Current component signature:

const Component = (props, children) => (
  <div {...props}>{children}</div>
)

Desirable component signature:

const Component = (props) => (
  <div {...props}>{props.children}</div>
)
// or even
const Component = (props) => (
  <div {...props} />
)

Props

• You may omit the unused argument if your component uses only children, ref #567
  js const Component = (props) => ( <div>{props.children}</div> )
  vs
  js const Component = (_, children) => ( <div>{children}</div> )
• It is easier to use memoization because you don't need to compare children separately from other props
  ```js
  const Component = (props) => (
  

  
  )

const MemoizedComponent = memoize(
Component,
// by default (props, nextProps) => !shallowEqual(props, nextProps)
)
const DeepMemoizedComponent = memoize(
Component,
(props, nextProps) => !_.deepEqual(props, nextProps)
)
const CustomMemoizedComponent = memoize(
Component,
(props, nextProps) => props.children !== nextProps.children
)
vs js
const Component = (props, children) => (

)

const MemoizedComponent = memoize(
Component,
// by default (props, nextProps, children, nextChildren) => !shallowEqual(props, nextProps) || children !== nextChildren
)
const DeepMemoizedComponent = memoize(
Component,
(props, nextProps, children, nextChildren) => !_.deepEqual(props, nextProps) || children !== nextChildren
)
const CustomMemoizedComponent = memoize(
Component,
(_, _, children, nextChildren) => children !== nextChildren
)
```

• Using children prop is valid and it will not throw an error:
  TypeError: Cannot assign to read only property 'children' of object '#<HTMLHeadingElement>'
• It will be easier to add something new in the future (or via HOA)
  js const ComponentWithContext = (props, context) => ( <div>{props.children}</div> ) // Context for SSR aka context in React.js const ComponentWithConnect = (props, store, actions) => ( <div>{props.children}</div> ) // aka React-Redux Connect // etc.

Cons

• It is a breaking change and requires at least Hyperapp v2 release

[okwolf]

@frenzzy I wrote a userland solution to this as a wrapper around h (in cra-hyperapp) for better compatibility with existing React components written in JSX: https://github.com/okwolf/cra-hyperapp/blob/2a246370cf6f62b671fb0d687c98dcaf85e3f2d9/src/index.js#L7

[Mytrill]

Would it be ok to to have the children passed both as a second parameter and in the props?
That provides almost all the listed benefits AND keeps it backward compatible.
The only benefits that it removes is the last one (easier to add things with HOAs).

The drawback is that there are 2 ways to write components with children.

[jorgebucaran]

@okwolf This would also make "render props" easier to implement.

[SkaterDad]

Does this only affect JSX?

I use h directly, so I'm not sure this affects me at all.

[frenzzy]

@SkaterDad it affects you only if you are using components pattern to encapsulate and reuse parts of your application:

const Component = (props, children) => h('div', props, children)
const Component = (_, children) => h('div', { title: 'x' }, children)
// vs
const Component = (props) => h('div', props, props.children)
const Component = (props) => h('div', { title: 'x' }, props.children)

h(Component, {}, ['Hi'])

[Swizz]

I am in favor of both, this relate what I had proposed about the (d, s, a) => (s, a) to allow people to use one or another feature.
And this cost only 2 bytes by parameter.

[SkaterDad]

@frenzzy Thanks for clarifying. I'm not using that pattern right now, so I'm good with whatever you all decide here!

[zaceno]

@JorgeBucaran I'm not really sure. In my very ignorant opinion, neither the pros or the cons (in the OP) are especially strong arguments. In such cases I tend to lean toward no change . But I'm basically fine with whatever.

As for the "both" suggestion... I think it should be either or -- otherwise it looks bad. Like we don't have a clear idea/agreement.

If we do decide definitely that 2.0 will have props.children, then it would be ok until that time, to have the signature ({children: ...}, children) as long as we deprecate the second arg in the docs. (Making it clear that we've made a concious decision, but leaving it there for compatibilty until 2.0)

[jorgebucaran]

@frenzzy Can you file a PR with your proposed changes?

I am thinking something along these lines:

attributes = attributes || { children: children }

...

[frenzzy]

If you solve the issue by mutating arguments:

attributes = Object.assign({}, attributes, { children: children })

https://github.com/hyperapp/hyperapp/blob/c1a22ae45817f681a43059db27730d82d2704632/src/index.js#L20
then the simple props comparison check (props === prevProps) will always return false which is not optimal.

Userland workaround for this issue is to use

<Component children={<Child />} />

instead of

<Component><Child /></Component>

[jorgebucaran]

@frenzzy So far so good, but I didn't understand this part:

then the simple props comparison check (props === prevProps) will always return false which is not optimal

What are you referring to?

[frenzzy]

I mean this:

let prevProps
const Component = (props) => {
  if (props === prevProps) {
    console.log('This will never happen if always create a new props object')
  }
  prevProps = props
  return <h2>Hello world!</h2>
}

const state = {
  count: 0,
  example: { deep: 'value' }
}

const actions = {
  up: () => state => ({ count: state.count + 1 })
}

const view = (state, actions) => (
  <main>
    <h1>{state.count}</h1>
    <button onclick={actions.up}>+</button>
    <Component {...state.example} />
  </main>
)

app(state, actions, view, document.body)

CodePen: https://codepen.io/frenzzy/pen/GQyWRK?editors=0010

[jorgebucaran]

@frenzzy If I run your example, I get that message logged to the console every time. That's the current behavior.

My question is then, what behavior do you want?

[frenzzy]

Current behavior is the best, because it will not require to prop keys comparison for memoization, just ===

[jorgebucaran]

@frenzzy To clarify, the current behavior, as defined in the h function, is to pass the attributes to the component as is. That's what you want, yes?

[frenzzy]

yes

[frenzzy]

so, let's close this?

[frenzzy]

I think there are no way to reach desired behavior without downsides (without mutating attributes which breaks memoization).

You can just pass children into your component as attribute to reach the desired effect.

[jorgebucaran]

@frenzzy I was re-reading your previous comments and got confused about what you were saying here:

You are not mutating the arguments, this creates a new object, so what do you mean?

We have two options:

1. Create new object like you said above.
2. _Mutate_ the attributes, adding a new children prop to it.
   
   
   
   • I think this requires two changes in h.js
     
     
     
     
     
     1. delete attributes.children and
     
     
     
     2. See below:
        
        
        js if (typeof name === "function") { attributes = attributes || {} attributes.children = children return name(attributes) } else { return { nodeName: name, attributes: attributes || {}, children: children, key: attributes && attributes.key } }
     
     
     
   
   

(2) Looks good to me, what am I missing?

[frenzzy]

1. Create new object like you said above.

On every render call the component will receive new attributes object even if user keep them in the application state (as in example below). You won't able to do simple === check, you have to do shallow compare previous and new attributes object which is slower.

1. _Mutate_ the attributes, adding a new children prop to it.

If attributes comes from state, than you can accidentally mutate application state too.

const state = {
  count: { type: 'number', min: 0, max: 100, step: 10, value: 10  }
}
const view = (state) => <input {...state.count} />
app(state, {}, view)
// TypeError:
// Cannot assign to read only property 'children' of object '#<HTMLButtonElement>'

plus unexpected behavior for user: "why I receive this error?"

[okwolf]

Seems like #696 might close this?