Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reverse the render order #7

Closed
jamesplease opened this issue Jan 22, 2018 · 9 comments · Fixed by #33
Closed

Reverse the render order #7

jamesplease opened this issue Jan 22, 2018 · 9 comments · Fixed by #33
Labels

Comments

@jamesplease
Copy link
Owner

jamesplease commented Jan 22, 2018

Should the first component listed be the outermost?


Libs with right-to-left as the only behavior or the default behavior:

  • Redux
  • Apollo
  • React Request (presently)

Libs with left-to-right as the only behavior or the default behavior:

  • Lodash
@jamesplease jamesplease changed the title Reverse the render order? Reverse the render order Feb 3, 2018
@jamesplease
Copy link
Owner Author

jamesplease commented Feb 3, 2018

I plan to do this to make "serial mode" (#19 ) easier to implement

@jamesplease jamesplease added enhancement New feature or request good first issue Good for newcomers and removed question enhancement New feature or request labels Feb 3, 2018
@TrevorSayre
Copy link

TrevorSayre commented Feb 4, 2018

I prefer the use of RTL as you have it. It is more comfortable coming from a functional programming background and fits in nicely with the general push towards more functional programming thanks to Redux, Ramda, etc. That said, if it helps you develop other important features more easily then go for it!

As I type in the parameters I'm thinking, "what's at the lowest level? okay, what is their parent? and theirs?" I find it sensible to think of the children first ;)

@jamesplease
Copy link
Owner Author

Thanks for the feedback, @TrevorSayre ! Just to share a little more insight into why I was thinking of reversing it, I think there is value in a mode where each component gets passed the results of the past one. This allows for some very sophisticated composition of function components (as an example, serial HTTP requests).

If the order were reversed from how it is now, then the API for that system would look like this:

<Composer
  functional
  components={[
    () => <RenderPropComponent {...configOne} />,
    (resultOne) => <RenderPropComponent {...configTwo} />,
    (resultOne, resultTwo) => <RenderPropComponent {...configThree} />
  ]}>
  {([resultOne, resultTwo, resultThree]) => (
    <MyComponent results={[resultOne, resultTwo, resultThree]} />
  )}
</Composer>

With the current render order, the API would need to be:

<Composer
  functional
  components={[
    (resultThree, resultTwo) => <RenderPropComponent {...configOne} />,
    (resultThree) => <RenderPropComponent {...configTwo} />,
    () => <RenderPropComponent {...configThree} />
  ]}>
  {([resultOne, resultTwo, resultThree]) => (
    <MyComponent results={[resultOne, resultTwo, resultThree]} />
  )}
</Composer>

which is strange, I think. Or, I could reverse the rendering order when functional is passed. But that seems like it could be unexpected to flip the order based on this prop.

Or a new prop could be added, reverse, that toggles the render order. What I don't like about this is that it seems like prop bloat.

Anyway, I'm just rambling 🙂 . I don't have any concrete plans to make a change to this component right now. Since this lib is getting more attention, I might wait it out to get a sense of what others are thinking.

Thanks again for the input @TrevorSayre ! Also, long time no see, man! I think it's been almost 4 years! I hope things are good ✌️

@gnapse
Copy link

gnapse commented Feb 6, 2018

I too think that the order of the array of components should be from outer to inner components.

@erikthedeveloper
Copy link
Collaborator

Initially, I was leaning toward RTL first:Inner and last:Outer, but after thinking about it a lot more (while looking into #28) it feels clearer and makes more sense to me as LTR first:Outer and last:Inner

When composing higher order components using RTL such as compose or flowRight, while the HoC invocation order is RTL, the resulting component hierarchy does end up being first:Outer and last:Inner. Example

const Enhanced = flowRight([
  wrapLastRenderFirstAsOuter,
  wrapFirstRenderLastAsInner,
])(SomeComponentRenderedAtVeryBottom)

For me, the clarity of LTR first:Outer / last:Inner becomes more apparent when discussing Support passing results of outer component(s) to inner component(s) #28.

<Composer
  components={[
    <Outer {...outerProps} />,
    <Middle {...middleProps} />,
    ([outerResult, middleResult]) => <InnerNeedsResults {...outerResult} {...middleResult} />,
  ]}>
  {([outerResult, middleResult, innerResult]) => (
    <MyComponent results={[outerResult, middleResult, innerResult]} />
  )}
</Composer>

📝 As I've been thinking and writing about this, I realized just how confusing it can be in either case with RTL or LTR ordering. I might suggest using language such as "outer", "middle" , "inner" in the README and examples. That feels more explicit/clear to me and eliminates a bit of mental parsing (especially for those not super familiar with composition in general and especially within React).

@gnapse
Copy link

gnapse commented Feb 8, 2018

+1 to not talk about first or last in render order, but about outer and inner in the nesting of the resulting composition.

@jamesplease
Copy link
Owner Author

@erikthedeveloper would you like to put together a PR to change the render order? ✌️

@erikthedeveloper
Copy link
Collaborator

I know this has already been completed/merged in #33, but what I found interesting was that pre-#33 even though the hierarchy was "reversed" as first:Inner, last:Outer the evaluation and "results accumulation" was actually happening so that the first:Outer, last:Inner functional API we were wanting was already possible/working as merged in #29 enabling this:

<Composer
  components={[
    // Simple elements may be passed where previous results are not required.
    <Outer />,
    // A function may be passed that will be invoked with the currently accumulated results.
    // Functions provided must return a valid React element.
    ([outerResults]) => <Middle results={[outerResults]} />,
    ([outerResults, middleResults]) => (
      <Inner results={[outerResults, middleResults]} />
    )
  ]}>
  {([outerResults, middleResults, innerResults]) => {
    /* ... */
  }}
</Composer>

Sort of a mind bender, since even though Inner was really on the outermost layer of the resulting component hierarchy, it had access to and could depend on Outer's results even though Outer was on the inner most layer 🤔...

@jamesplease
Copy link
Owner Author

jamesplease commented Feb 9, 2018

ahahaha @erikthedeveloper that is one of the reasons I PR'd to reverse the order so quickly after releasing that version. The function feature made the old rendering order super weird, particularly with the terminology "outer" and "inner" 😉

I think we're in a great place now, though. Thanks so much for all of your help! ✌️

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

Successfully merging a pull request may close this issue.

4 participants