Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

Improve fromRenderProps #702

Open
TiuSh opened this issue Jul 11, 2018 · 4 comments
Open

Improve fromRenderProps #702

TiuSh opened this issue Jul 11, 2018 · 4 comments

Comments

@TiuSh
Copy link

TiuSh commented Jul 11, 2018

The new feature fromRenderProps looks really nice, but I think it would be even better if we could send props to the inner renderProp component like that:

const RenderPropComponent = ({ name, children }) => children({ text: `Hello ${name}!` });
const component = ({ text }) => <h1>{text}</h1>;

const EnhancedComponent = fromRenderProps(
  RenderPropComponent,
  // or:
  // ({ name, children }) => <RenderPropComponent name={name} children={children} />,
  ({ text }) => ({ text })
)(component);

<EnhancedComponent name={John} />
// Would render <h1>Hello John!</h1>

It would be as simple as adding ...ownerProps to fromRenderProps.js (L14):

  const FromRenderProps = ownerProps =>
    renderPropsFactory({
      ...ownerProps,
      [renderPropName]: (...props) =>
        baseFactory({ ...ownerProps, ...propsMapper(...props) }),
    })

I deal with a lot of "render prop" components, and I won't be able to use fromRenderProps with most of them until such a change has been made.

What do you think about that ? I will be more than happy to send a PR if needed !

@wuct
Copy link
Contributor

wuct commented Jul 12, 2018

Considering providing a more general solution, we can change the third argument of fromRenderProps from

renderPropsName :: String

to

renderPropsName :: String
| mapPropsToRenderPropsProps :: OwnerProps -> RenderPropsProps

As a result we can use it like

const EnhancedComponent = fromRenderProps(
  RenderPropComponent,
  ({ text }) => ({ text }),
  ({ name, children }) => ({ name, children }) // or just an identity function
)(component);

However, in my opinion

const EnhancedComponent = fromRenderProps(
  ({ name, children }) => <RenderPropComponent name={name} children={children} />,
  ({ text }) => ({ text })
)(component);

is good enough currently :)

@TiuSh
Copy link
Author

TiuSh commented Jul 12, 2018

Actually I really like your idea to change renderPropsName, but with a small tweak:

String | (OwnerProps, childrenFn) -> RenderPropsProps
or
String | (childrenFn, OnwerProps) -> RenderPropsProps

As it would be more explicit on which props are sent to the renderProp component, and we won't risk to overload an outter children prop. Btw the signature would recall lodash way of doing things (with the iteratee shorthands: https://lodash.com/docs/4.17.10#map).

@TiuSh
Copy link
Author

TiuSh commented Jul 12, 2018

This way my example would change to:

const RenderPropComponent = ({ name, children }) => children({ text: `Hello ${name}!` });
const component = ({ text }) => <h1>{text}</h1>;

const EnhancedComponent = fromRenderProps(
  RenderPropComponent,
  ({ text }) => ({ text }),
  ({ name }, children) => ({ name, children })
  // or:
  // (children, { name }) => ({ name, children })
)(component);

<EnhancedComponent name="John" />
// Would render <h1>Hello John!</h1>

@TiuSh
Copy link
Author

TiuSh commented Jul 23, 2018

Any update about this issue ? If anyone is ok with it I could easily submit a PR !

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

No branches or pull requests

2 participants