Wp-calypso: Composing Higher Order Components

[spen]

Hi All 👋

I wanted to talk about how we are currently 'composing' higher-order components (HOCs).

Currently, we see this a lot...

export default connect( ... )( localize( SomeComponent ) );

Which is manageable, sure.

The problem though is that it's not a pattern that 'scales' well as more HOCs are introduced...

export default connect( ... )( localize( someHoc( someOtherHoc( SomeComponent ) ) ) );

As mixins are phased out from components and HOCs become more commonly used, the current pattern will end up looking quite cumbersome.

I've opened a PR (#10137) to show one component that would need 3 HOCs and how redux's compose method allows for a nicer, more declarative and more 'functional' syntax.

Here's the gist for anybody who may not already be aware of compose, refactoring the above example:

// note that HOCs will be applied from bottom to top (right to left)
export default compose(
    connect( ... ), // Applied last
    localize, // ☝
    someHoc, // ☝
    someOtherHoc, // ☝ Applied first
)( SomeComponent );

I would suggest that going forward, the compose pattern is favoured in any case where 2 or more HOCs are used to wrap the component.

I'm interested in others thoughts on this - is it more reasonable to only apply once 3 or more HOCs are used? Should we add something to the guidelines to address how HOCs are composed?

[gziolo]

Thanks for opening this discussion :)

I wasn't aware that compose is a part of Redux's library API! To be honest, I would sooner expect to see it in react-redux.

If we want to start promoting composition of HOCs, we might consider using recompose. It also has also compose method and also lots of very useful HOCs available out of the box. @acdlite gave very interesting talk about it during React Europe earlier this year:

https://www.youtube.com/watch?v=zD_judE-bXk

I have just find out that the only difference between compose from recompose and compose from Redux is just that the latter works only with functions. BTW, flowRight from Lodash also uses the same API as aforementioned compose. In lodash/fp it's fixed and renamed to compose :)

I would suggest that going forward, the compose pattern is favoured in any case where 2 or more HOCs are used to wrap the component.

I'd love to see that coming. I think we should recommend using it starting from 2 HOCs, but I wouldn't enforce myself strict rule on that.

[youknowriad]

Thanks for bringing this up. We had a discussion with @aduth about this on this PR https://github.com/Automattic/wp-calypso/pull/9085#issuecomment-262187700 We used flowRight for now since lodash is our default "utils" library.

We could also consider normalizing the name fo the ReduxConnect HoC call as connectComponent.

IMO, 2 HOCs is acceptable without composing but I think composing should be recommended for more.

[gziolo]

We used flowRight for now since lodash is our default "utils" library.

We can also use:

import compose from 'lodash/fp/compose';

[spen]

@youknowriad Thanks for the background! I see why redux.compose is not quite ideal now!
I personally like compose over flowRight, it feels a little more semantic given the context.

[youknowriad]

I personally don't have a strong preference for one over another, Though, I also think the compose name is better. so 👍 for import { compose } from 'lodash/fp'.

I'll update the PR once a consensus is reached ;)

[aduth]

Can someone explain why lodash/fp/compose would be preferable over lodash/flow or lodash/flowRight ? A developer could always rename the import via import { flowRight as compose } from 'lodash'; if really concerned about the naming. Just seems to me that introducing parts of lodash/fp could be confusing for a new developer. It also seems less stable and bloat-inducing given this note on the Lodash roadmap:

Move lodash/fp back to lodash-fp (35% savings)

[youknowriad]

Right! this is really concerning, we don't want to add an entire new npm library for a function that is already available on an existing one.

[dmsnell]

I'd be pretty resistant to introducing lodash/fp because I have found that it introduces a lot of confusion over the calling conventions compared to lodash the first.

Something like ramda was build from the ground up to be _iteratee-first_, _data-last_, and _auto-curried_ and that all feels just right. However, in my experimentation with lodash/fp I just can't ever seem to get it right. By tying strongly to lodash it has to try and fit that paradigm while handling functions that were designed and used as _data-first_ and _iteratee-last_ and worse, a host of functions with optional parameters (which don't jive well with auto-currying).

It could get better, but we end up with duplicate functions within lodash that have arguments in backwards orders and I couldn't effectively use it without the big list of functions mapping to their rearranged argument order.

In other words, @aduth, this is why I think flowRight is preferable to lodash/fp#compose

[aduth]

In other words, @aduth, this is why I think flowRight is preferable to lodash/fp#compose

To clarify, my argument was in favor of using lodash/flowRight.

[spen]

I've given this a bit more thought, here's a sort of summary of what I'm thinking right now...

Redux compose ❌

I think this _almost_ feels right but only because in the majority of cases where we have multiple HoCs to compose connect is one of them, which at least relates to redux.
Although it might _feel_ right in those cases it certainly wouldn't make sense in the odd case where the component isn't connected but utilizes other HoCs... If the day comes when redux is to be replaced, we'd be relying on it purely for a very simple util method, and then there's the concern of redux dropping or changing compose.
I think this is in the same vein as @aduth 's comment on #9085.

I'd rule this out as it's not Redux's responsibility here.

lodash/fp/compose ❌

Great points here from @aduth & @dmsnell
That extra cost, just for the sake of a slightly better named function, would not be worth it!

A related side-note - I could be wrong, but I've read that destructuring libraries like so:

import { map, reduce } from 'lodash'; 

// vs 

import map from 'lodash/map';
import reduce from 'lodash/reduce';

Causes the full library to be bundled, which I think would mean that 'lodash/fp' is already bundled.
Again I may be wrong as I've not explored this or compared bundle sizes.
It's also not a good reason to use lodash/fp/compose, just bringing it up as it might be something worth exploring more.

The extra load, or at least the extra tie-in to that extra load, rules this option out IMO.

lodash/flowRight ✅

My issue here is only with the name which really isn't important, 'flowRight' is still very expressive, I just think that compose better describes what we're trying to achieve, to 'compose' a component.

@aduth, you mention simply aliasing the import to 'compose'.
I'm on the fence with this idea personally... Would the aliasing cause any confusion? or, conversely, does it show the intent better?
I'd rather the code itself be clearer to somebody approaching it than the import so I think I'm leaning towards the idea of aliasing the import.

My ➕☝ would be lodash/flowRight - As mentioned, lodash is the projects util library, it's fitting then that lodash handles this.

Thanks for the discussion so far guys! 😀

[aduth]

A related side-note - I could be wrong, but I've read that destructuring libraries like so: _[...]_ Causes the full library to be bundled, which I think would mean that 'lodash/fp' is already bundled.

This is normally true, but we include babel-plugin-lodash in our build process which automagically converts import { map } from 'lodash'; into import map from 'lodash/map';

I'm on the fence with this idea personally... Would the aliasing cause any confusion? or, conversely, does it show the intent better?

I don't have a good answer to this. I get the sense that "compose" is a generally more familiar term for developers than "flowRight", but renaming imports is uncommon and in use would be a convention that new developers would need to learn through encountering.

[dmsnell]

Causes the full library to be bundled, which I think would mean that 'lodash/fp' is already bundled.
Again I may be wrong as I've not explored this or compared bundle sizes.
It's also not a good reason to use lodash/fp/compose, just bringing it up as it might be something worth exploring more.

we have a transform in the build step that pulls out the top-level imports into the minified version. concurrently, we are questioning pulling all of lodash in anyway as a webpack extern. in any case, writing import { map } from 'lodash' in Calypso is equivalent to writing import map from 'lodash/map'.

aliasing

we already have several of us using import { flowRight as compose } from 'lodash' inside of Calypso and I don't see any problem with that. it's unfortunate that lodash named it flowRight because of how the rest of the programming world settled on compose years ago ¯\_(ツ)_/¯

[spen]

but we include babel-plugin-lodash in our build process

Did not know this, very neat :D

we already have several of us using import { flowRight as compose } from 'lodash'
Since that's the case I'm happy to swing my vote that way!

I think I have one or two PRs open that use Redux's compose - I'll update those and open another to change my recently landed PRs that also use Redux's compose a little later on :)

[gziolo]

it's unfortunate that lodash named it flowRight because of how the rest of the programming world settled on compose years ago ¯\_(ツ)_/¯

Yes, I agree that using flowRight could be named differently, but it seems like a best pick in this context :)