Update props.components as functions to component({results, render}) #39

erikthedeveloper · 2018-02-13T01:34:28Z

These changes give all power/flexibility to the props.components as function entries which essentially become render prop functions themselves.

📝 This is a breaking change

This reduces API surface area and covers advanced use cases such as composing components with differing render prop names as discussed in #37 and multi-argument producers.

Example covering all major use cases (also see tests for further coverage/examples):

<Composer
  components={[
    // React elements may be passed for simple/basic use cases.
    // Assumes single value produced such as `props.children(producedValue)`
    <Echo value="outer" />,

    // OR A function may be passed to produce an element.

    // Utilizing outer results for inner components
    ({ render, results: [outerResult] }) => (
      <Echo value={`${outerResult.value} + middle`} children={render} />
    ),
    
    // Differing render prop signature (multi-arg producers)
    ({ /* results, */ render }) => (
      <DoubleEcho value="spaghetti">
        {(one, two) => render([one, two])}
      </DoubleEcho>
    ),
    
    // Differing render prop names...
    ({ /* results, */ render }) => (
      <EchoRenderProp value="spaghetti" renderProp={render} />
    )
    ]}
  ]}
  children={/* ... */}
/>

TODO

Update tests to reflect new behavior
Change the signature from components[n](results) to components[n]({results, render})
Eliminate props.renderPropName and props.mapResult. All use cases now covered by props.components as render functions
Refactor Composer, extract renderRecursive outside of render
- 📝 Not entirely necessary, but with index.js being such a small module already, this refactor sort of just happened during the process...
Require props.children and props.components (📝 not totally necessary, but considering this is already a breaking change tempting to squeeze in)
Update docs (see https://github.com/jamesplease/react-composer/blob/a77449db49ed238d5c97526a1e803c8094514c1d/README.md)

This originally came up in #38 see discussion here #38 (comment)

erikthedeveloper · 2018-02-13T01:36:41Z

@jmeas this is me playing with the idea from #38 further. Let me know what you think of this general direction. I know it is breaking, but I feel like it could be a good API and leave flexibility/power up to consumers via those render functions.

jamesplease · 2018-02-13T05:57:16Z

Hey @erikthedeveloper ! Thanks again for the great PR.

I agree with you about the benefits of this PR. This is clearly the most powerful option we have discussed for the API, and I like how it neatly handles several of the situations we've been thinking about lately with this one change.

With that said, I'm not completely sold on making this change right now. I am a little bit hesitant about the API churn associated with dropping a breaking change like this so soon after adding function support.

What would motivate me more to do the breaking change sooner would be if this supported some super obvious and common use case that Composer just couldn't possibly support right now. But I'm not sure what that use case is right now. What do you think? Do you need this feature right now?

I'm wondering if the best thing to do is to hold off on this for the time being. Composer works well for the majority of use cases that I think developers will run into.

One other thing giving me pause is that the last time I made a change without a clear motivation (adding mapResults), I regretted it.

Plus, not merging this now doesn't mean that we never will. I am definitely open to this change. I think it is very cool. I just want to know that someone actually needs it, you know?

I'm curious what you think. (I'm not 100% convinced of anything, by the way. I'm just sharing my thoughts 💭 )

And as a reply to some of the footnotes you posted:

Note that transform-react-remove-prop-types doesn't seem to really be working

Hrm, what are you seeing? Here is a screenshot comparing react-composer.js (which doesn't use that plugin), and a beautified react-composer.min.js, which does:

Click to expand

All of that code on the left is the prop-types code, so it seems to be working for me. Are you seeing prop-types code within react-composer.min.js?

*I've been running into odd build issues on my local with node 6.12.3

Hmmm, interesting. What sorts of things have you been seeing? I wonder if the problems exist on 8.x as well. I'm using 8.x here and I haven't noticed anything, although it's entirely possible that there is some bug with the build system that I just haven't run into yet 🤔

erikthedeveloper · 2018-02-14T23:38:43Z

Maybe my build problems are node 6.x specific. They were seemingly sporadic which I found doubly odd. I somehow narrowed it down to the prop types removal happening through the babel config option provided for buildProd or something odd 🤔 haha... I'm fairly certain its related to my local setup but I haven't really isolated the problem yet.

I see what you are saying about avoiding unnecessary API churn and have to agree. Maybe it's good to let this alternative API stew a bit and if it still makes sense down the road we can explore further.

jamesplease · 2018-02-17T18:22:45Z

I'm fairly certain its related to my local setup but I haven't really isolated the problem yet.

Alright, cool. Well, let me know if you think this is a problem on the build setup side, and I'd be happy to take a look.

Maybe it's good to let this alternative API stew a bit and if it still makes sense down the road we can explore further.

Cool. I'm going to close this out for now, but if you / anyone else reads this and really wants it merged, please leave a comment here and we will reopen this and continue the discussion!

jamesplease · 2018-03-04T20:20:13Z

I opened a poll for whether or not we should merge this over in #43 . Future developers: if you would like to see this, please leave your vote over in that issue! Thanks! ✌️

jamesplease · 2018-03-12T23:37:31Z

I've reopened this, because based on the poll in #43 people are favorable to this change ✌️

jamesplease · 2018-03-20T20:34:47Z

@erikthedeveloper I am thinking this Saturday I'll merge and release this as v5.0.0. What do you think?

erikthedeveloper · 2018-03-21T02:27:30Z

That sounds good to me @jamesplease. I can work up and push README commit tonight or tomorrow and maybe you can circle back and highlight/update the README as you see fit.

erikthedeveloper · 2018-03-21T03:36:09Z

I've got this initial README commit pushed up 8dd407b. Let me know what you think.

Dang, I forgot to merge in master first so there are some conflicts. I'll have to revisit that tomorrow. They look fairly minor (mostly just the username change I hope 🙏).

erikthedeveloper · 2018-03-21T13:37:14Z

Ok @jamesplease I've got those conflicts resolved. See the current README here: https://github.com/jamesplease/react-composer/blob/a77449db49ed238d5c97526a1e803c8094514c1d/README.md I added a section for props.components as render functions along with some examples per use case.

jamesplease · 2018-03-22T04:28:42Z

@erikthedeveloper this all looks really great! The docs updates are 👌 I am thinking this is good to merge without any changes.

As an aside, if you are interested I could add you as an npm maintainer, and you could cut the release. Would you be interested in that, or nah? (No worries either way)

erikthedeveloper · 2018-03-22T15:35:48Z

Thanks @jamesplease! I did push up one minor commit after taking a fresh look while responding to #47 (Shuffle renderRecursive signature ffa3f8a). But yeah, let's get this merged and shipped as v5.0.0 then 😃

Outside the scope of this PR, but I'm not sure if it makes sense to add me as an npm maintainer right now. I'm still running into issues building locally and actually can't get it to build at all right now so I would be unable to release anyhow 😕😆 I tried with node v6 and v9 via n and I haven't run into any other troubles outside of trying to build react-composer.

The problem seems to be triggered only while attempting build:umd:min.

> react-composer@4.1.0 build:umd:min /Users/eaybar/Code/github/jmeas/react-composer
> cross-env NODE_ENV=production BABEL_ENV=buildProd rollup -c -i src/index.js -o dist/react-composer.min.js

🚨   (babel plugin) RangeError: /Users/eaybar/Code/github/jmeas/react-composer/src/index.js: Maximum call stack size exceeded
src/index.js

I might have to investigate this another time and see if it is just a problem with my local setup or something potentially rollup/babel related.

jamesplease · 2018-03-22T16:36:44Z

I did push up one minor commit after taking a fresh look while responding to #47 (Shuffle renderRecursive signature ffa3f8a). But yeah, let's get this merged and shipped as v5.0.0 then 😃

Sounds good ✌️

I might have to investigate this another time and see if it is just a problem with my local setup or something potentially rollup/babel related.

Ah, right, I remember you mentioning those problems. Alright, no problem, I'll cut the release.

- Update tests to reflect new behavior - Refactor Composer, extract renderRecursive outside of render - Require props.children and props.components. - Eliminate props.renderPropName and props.mapResult - Leave a note that transform-react-remove-prop-types doesn't seem to really be working - *I've been running into odd build issues on my local with node 6.12.3

- Shuffle arg order from (remaining, results, render) to (render, remaining, results) - Set default arg for initial results - Use `results = results || []` instead of ES6 default arg `results = []` to reduce filesize ```diff -renderRecursive(props.components, [], props.children); +renderRecursive(props.children, props.components); ```

jamesplease · 2018-03-23T01:21:18Z

@erikthedeveloper I am also getting those build errors now. I think it is a regression in Rollup. Because the deps aren't locked down here, you must have installed a version after the regression was introduced. I just reinstalled the deps and it started happening to me, too.

I opened an issue over in rollup/rollup#2085 to track the problem. For now, I am going to remove the offending plugin (the one that removes prop types). This is unfortunate because it will bloat the file size, but we can do a v5.0.1 that removes it once the Rollup configuration is working again.

jamesplease · 2018-03-23T01:24:56Z

v5.0.0 has been published! Thanks for the awesome work here @erikthedeveloper ✌️

erikthedeveloper added enhancement New feature or request breaking work in progress labels Feb 13, 2018

erikthedeveloper changed the title ~~Refactor props.components as functions to component({results, render})~~ Update props.components as functions to component({results, render}) Feb 13, 2018

jamesplease closed this Feb 17, 2018

erikthedeveloper mentioned this pull request Mar 4, 2018

PropTypes error #42

Closed

jamesplease mentioned this pull request Mar 4, 2018

Poll: breaking API change? #43

Closed

jamesplease reopened this Mar 12, 2018

jamesplease mentioned this pull request Mar 22, 2018

Exposal of chainComponents #47

Closed

erikthedeveloper added 5 commits March 22, 2018 17:49

Update README for new components as functions API

6e56107

Fix mention of props.render w/ props.children

1fe7224

Remove random TODO comment

0bc3c5d

jamesplease force-pushed the render-props-for-render-props branch from ffa3f8a to 3e86c07 Compare March 23, 2018 00:51

jamesplease added 2 commits March 22, 2018 17:53

Update README

e22b5e1

5.0.0

f608f03

jamesplease merged commit 2b267ae into master Mar 23, 2018

jamesplease deleted the render-props-for-render-props branch March 23, 2018 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update props.components as functions to component({results, render}) #39

Update props.components as functions to component({results, render}) #39

erikthedeveloper commented Feb 13, 2018 •

edited

Loading

erikthedeveloper commented Feb 13, 2018

jamesplease commented Feb 13, 2018 •

edited

Loading

erikthedeveloper commented Feb 14, 2018

jamesplease commented Feb 17, 2018 •

edited

Loading

jamesplease commented Mar 4, 2018

jamesplease commented Mar 12, 2018

jamesplease commented Mar 20, 2018

erikthedeveloper commented Mar 21, 2018

erikthedeveloper commented Mar 21, 2018

erikthedeveloper commented Mar 21, 2018

jamesplease commented Mar 22, 2018

erikthedeveloper commented Mar 22, 2018

jamesplease commented Mar 22, 2018

jamesplease commented Mar 23, 2018

jamesplease commented Mar 23, 2018

Update props.components as functions to component({results, render}) #39

Update props.components as functions to component({results, render}) #39

Conversation

erikthedeveloper commented Feb 13, 2018 • edited Loading

TODO

erikthedeveloper commented Feb 13, 2018

jamesplease commented Feb 13, 2018 • edited Loading

erikthedeveloper commented Feb 14, 2018

jamesplease commented Feb 17, 2018 • edited Loading

jamesplease commented Mar 4, 2018

jamesplease commented Mar 12, 2018

jamesplease commented Mar 20, 2018

erikthedeveloper commented Mar 21, 2018

erikthedeveloper commented Mar 21, 2018

erikthedeveloper commented Mar 21, 2018

jamesplease commented Mar 22, 2018

erikthedeveloper commented Mar 22, 2018

jamesplease commented Mar 22, 2018

jamesplease commented Mar 23, 2018

jamesplease commented Mar 23, 2018

erikthedeveloper commented Feb 13, 2018 •

edited

Loading

jamesplease commented Feb 13, 2018 •

edited

Loading

jamesplease commented Feb 17, 2018 •

edited

Loading