render-props-compose icon indicating copy to clipboard operation
render-props-compose copied to clipboard

Comparing with React Composer

Open jamesplease opened this issue 7 years ago • 9 comments

Hi there! Really cool lib here! I recently made a similar lib, React Composer. Maybe we can join efforts and unify the community around one component?

If not, no worries. Thanks for reading :v:

jamesplease avatar Feb 06 '18 17:02 jamesplease

Wow, nice! I started this one recently as well, and yes, I'm definitely open to join efforts. How do you propose we could go forward?

I don't mind ditching mine for yours, given that it already seems more visible (somehow I missed it). Let me take a look at the feature differences more in depth to see how we could approach this. Or let me know if you already have some insights on this.

gnapse avatar Feb 06 '18 18:02 gnapse

Cool! I took a quick look to see some differences. Here's what I saw:

  • render-props-compose already supports object syntax. I have it as an open issue ( https://github.com/jmeas/react-composer/issues/23 )

  • React Composer does not implement a utility like composed

  • A quick look suggests mine renders in the opposite order, though I was thinking of reversing it ( https://github.com/jmeas/react-composer/issues/7 ) to support a new prop tentatively named functional ( https://github.com/jmeas/react-composer/issues/19 )

jamesplease avatar Feb 06 '18 19:02 jamesplease

Yup, the revers thing is the first one I noticed, and was about to comment there. I too think it's better that way, and as you correctly expose in that issue, it will make it more natural to add a feature to allow each component in the chain to access the props of the previous one.

This issue is the hardest to overcome unless we get on the same page. Like you also say there, adding yet another prop to say if one wants it reversed or not is bordering on prop bloat. If you end up open to do it, I think we can merge. Not sure how you'll deal there with the breaking change. Perhaps just a major version number bump and the warning in the release for users of your lib to upgrade.

If we overcome that hurdle, I'm more than happy to try to make PRs to your project to add the other two features this lib already has. I might need to change the implementation internals quite a bit but I promise I won't break the external interface or the tests.

What do you think?

gnapse avatar Feb 06 '18 19:02 gnapse

My current plan is to:

  1. reverse the render order
  2. add some sort of prop that behaves like functional
  3. not add a reverse prop

This will be done in a major release, since it is a breaking change. And there will be an accompanying CHANGELOG entry describing the changes.

What do you think of this plan, @gnapse ?

jamesplease avatar Feb 06 '18 19:02 jamesplease

1 and 3 sounds good. I don't get what you mean with 2.

Back to our plans to join efforts: it'd have to be after 1, and depending on what the point 2 means, perhaps before it as well. I was taking a look at the implementation, and they seem radically different, even when the external result is mostly the same besides this order issue. Not sure how to merge my features in it.

gnapse avatar Feb 06 '18 19:02 gnapse

BTW, your README is inconsistent. The very first example of what the library does, does not seem to adhere to the reverse order. In the un-composed example you render from outer to inner configOne, configTwo and configThree, and then the composed example lists these in that same order.

gnapse avatar Feb 06 '18 19:02 gnapse

Another feature I'd like to export to your lib, the ability to specify components merely by name. If you don't need to pass any props to it, it's simpler to put just MyRenderPropWrapper than <MyRenderPropWrapper />. That's simple to port , and it's not a breaking change.

gnapse avatar Feb 06 '18 19:02 gnapse

@gnapse item 2 refers to what you described as:

a feature to allow each component in the chain to access the props of the previous one.

It can be read about more in-depth here: https://github.com/jmeas/react-composer/issues/19 , and is the primary motivation behind me swapping the render order.

Not sure how to merge my features in it.

One thing to note is that I'm not particularly interested in adding a HoC-like method like compose to React Composer. HoCs aren't hard for users to build in userland from a render prop component, and I think most developers will prefer the render prop API. If you think that is a requirement for merging, then they may have to remain as separate libs 🙂

BTW, your README is inconsistent.

The README accurately describes how the library currently behaves. The render order is last-to-first, which is to match the behavior of common functional utilities. However, the argument order respects the array that you pass in. This may seem weird, but until a prop like functional is added, the component is really only usable with components where the render order doesn't matter (like the new Context API).

Either way, this doesn't matter too much, I suppose, given that the order will be changing soon.

Another feature I'd like to export to your lib, the ability to specify components merely by name.

I'm not 100% convinced this one adds too much value. I'd be down to open an issue to get feedback from other users, but I wouldn't want to add it until it was clear that others agree that it is worth adding.


I'm definitely still down to merge, but our goals for our libs may be too different for merging. If so, that's OK! :v:

jamesplease avatar Feb 06 '18 19:02 jamesplease

I do want to merge efforts. I rather want my contributions to be useful and widely used than having living in my bubble. For starters, I don't have to ditch my project right away, but I'm more than happy to contribute my achievements to yours. I'll start with jmeas/react-composer#23.

That being said, the HoC is of course not the most important way to expose this, but I still see it has a value. I'll open an issue in your project. The best thing about it is that it does not need to break anything. It's an extra named export for those who want to use it.

gnapse avatar Feb 06 '18 19:02 gnapse