tie together setDisplayName+wrapDisplayName+getDisplayName

[cowchimp]

Hi.

setDisplayName, wrapDisplayName, and getDisplayName are powerful utilities.
But IMHO there should be a utility function that ties them all together for a very common scenario.

The most common pattern I see around naming HOCs is WrappingComponent(WrappedComponent)

so now, in order to give that name to WrappingComponent I need to use all three utility functions and to do:

import { setDisplayName, wrapDisplayName, getDisplayName } from 'recompose';

var newDisplayName = wrapDisplayName(WrappedComponent, getDisplayName(WrappingComponent));
setDisplayName(newDisplayName)(WrappingComponent);

So, two there are two things that are left to be desired here IMHO

1. That wrapDisplayName calls getDisplayName internally for WrappedComponent but not for WrappingComponent (which is why I have to use getDisplayName in the user code for that).
   This could be modified so that wrapDisplayName will be able to accept a component as the 2nd parameter as well (an overload of sorts), and internally call getDisplayName on it. This is a pretty low-risk breaking change because right now wrapDisplayName expects only a string, so modifying this should extend wrapDisplayName, not break it (and getDisplayName already returns a string if it gets a string).
2. That there will be a single method that accepts 2 components: the WrappingComponent reference and the WrappedComponent reference, will calculate the WrappingComponent(WrappedComponent) name and set it as the new name for WrappingComponent.
   This could be a new utility called setWrappedDisplayName`.
   For example:

import { setWrappedDisplayName } from 'recompose';

setWrappedDisplayName(WrappingComponent, WrappedComponent);
//the displayName of WrappingComponent is now WrappingComponent(WrappedComponent)

Would be happy to try to send in a PR if we can decide on the necessary changes.

Thanks!

[wuct]

Do you mind to describe what are WrappingComponents? Both getDisplayName and setDisplayName are designed to be used with a Component instead of a HOC/HOF. In other words, using getDisplayName on recompose's HOC/HOF will give you undefined.

[cowchimp]

Thanks for the response.

Can you take a look at this webpackbin?
http://www.webpackbin.com/41aSOoHrz

It's a contrived example where I have a presentational component that I want to wrap with a container component that will make it so that it will log to console when the base component is added to the DOM.

In this example LogComponentDidMount is the WrappingComponent and Greeting is the WrappedComponent

I'm using recompose so that the name of the container component in React DevTools to be LogComponentDidMount(Greeting).

So you see how in LogComponentDidMountHoc.js I had to use all three setDisplayName+wrapDisplayName+getDisplayName to achieve this?
That's my main issue.

LMK if I'm still being unclear.

Thanks.

[wuct]

Thanks for sharing your code! In your case, is there any concern to use a string directly instead of calling getDisplayname? This is how recompose does internally.

I mean changing your code from

let newDisplayName = wrapDisplayName(WrappedComponent, getDisplayName(LogComponentDidMount));

to

let newDisplayName = wrapDisplayName(WrappedComponent, 'LogComponentDidMount');

[cowchimp]

Thanks.

Well, maybe I'm nit picking, but there's a difference. Because in the 2nd case I would violate the DRY ("don't repeat yourself") principle. If I change the name of the class LogComponentDidMount I would have to remember to change the string 'LogComponentDidMount' as well.
I want to use getDisplayName() and pass the reference to the LogComponentDidMount class so that the name of the class is changed accordingly automatically for me and I don't have to worry about it.

Obviously, this is not a big deal for a single use-case in your code. But assume that you have many different custom HOCs. Calling all three of these functions together can be cumbersome.
That's why I think that a single function like setWrappedDisplayName that I proposed as a utility function would be valuable.

Makes sense?

Thanks.

[wuct]

@cowchimp It makes sense to me. Moreover, I think we can use setWrappedDisplayName in createHelper, and there is no need to pass a string name to createHelper anymore. Would you like to give it a try?

[cowchimp]

cool, I'll work on it.

Can you please explain why you avoid requiring wrapDisplayLine and setting displayName in production like you do here

recompose/src/packages/recompose/renderComponent.js

Line 7 in c68853e

if (process.env.NODE_ENV !== 'production') {

?
you also do something similar here

recompose/src/packages/recompose/createHelper.js

Line 7 in c68853e

if (process.env.NODE_ENV !== 'production' && setDisplayName) {

thanks.

[wuct]

It's used to increase performance and reduce the bundle size.

[jcheroske]

I would love to see the api for manipulating component names simplified in this way. Ever since I started using recompose (changed my React life, btw), I've wondered about what an api that handled the most common use-case would look like. The ideas discussed here seem right on target. Can't wait to see it.

[wuct]

@jcheroske glad to hear that Recompose can help you rethink React. I'm going to close this issue for now and a PR to refactor the code is still welcome.

[timkindberg]

I think we should leave this one open with a PR Welcome tag. It shouldn't be too hard.

On Tue, Feb 28, 2017 at 4:24 AM CT Wu ***@***.***> wrote: @jcheroske <https://github.com/jcheroske> glad to hear that Recompose can help you rethink React. I'm going to close this issue for now and a PR to refactor the code is still welcome. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#299 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKQblRpUM3arUibAkve_ymH6emBTaplks5rg-fZgaJpZM4LZ64W> .

-- - Tim

[jcheroske]

Can we have more of a discussion about what the community would like to see on this front? Specifically, heavy use of compose() results in deeply nested names that are difficult to read in the devtools. I feel like an enhancement to compose() is needed, that would apply a standard naming format across all the nested helpers. I really have no idea what that would look like, or if that's even feasible. Maybe something like:

<myAwesomeHOC-defaultProps(PlainComponent) ...
  <myAwesomeHoC-setPropTypes(PlainComponent) ...
    <myAwesomeHOC-withState(PlainComponent) ...

Just thinking out loud here...

[wuct]

The nested HOCs will be flattened after #182. And it would be better to open a new issue to discuss customized HOCs' names.

[jcheroske]

I just read #182. You guys are way ahead of me. I say close this and we can pick this up on the other side.

[timkindberg]

No I think we leave this still open. setWrappedDisplayName is still a totally valid feature request in my opinion. The HOC flattening stuff is not related.

[wuct]

I'm agree with @timkindberg. These are different requests.

[everdimension]

I think if wrapDisplayName function was curried AND accepted arguments in reverse order, it could be beautifully used like this:

import { connect } from 'react-redux';
import { compose, setDisplayName, wrapDisplayName } from 'recompose';
import * as actions from '../../actions/account';

export default compose(
  connect(state => ({
    account: state.account,
  }), actions),
  chain( // helper from an fp library like ramda
    setDisplayName,
    wrapDisplayName('withAccount'), // could be curried, but currently doesn't work like this
  ),
);

[timkindberg]

This is a very simple thing to add.

export const setWrappedDisplayName = (name) => (BaseComponent) => {
  return setDisplayName(wrapDisplayName(BaseComponent, name))(BaseComponent);
};

[everdimension]

@timkindberg I think more than half of recompose's hocs and helpers are easy to implement, but that's what this project is for — to be a toolbelt of helpful stuff that you might often wish to use.

So my comment was not a question of "how" but rather wondering whether this is not something that many people find themselves needing to have.

[timkindberg]

@everdimension true. Our team certainly needs it. I think it's very common and should be in the library.

[istarkov]

Having that it mostly debug enhancers why their are common?
Just curious.

[timkindberg]

Well think about it. Recompose is a toolbelt for writing our own Higher Order Components. HoCs need nice display names. Since HoCs wrap other components it is common that we need to set the wrapped display name for our custom HoCs. It is a trivial but important util to have in the library.

[wuct]

So what setWrappedDisplayName does is inserting a name into the displayName chain.

const enhance = compose(
  mapProps(/* do something */),
  setWrappedDisplayName('customizedName'),
  mapProps(/* do something */),
)(Foo)

Will show this in devtool: <mapProps(customizedName(mapProps(Foo))) />

But I think what you want is <mapProps(customizedName(Foo)) />, am I right?

[timkindberg]

That's a good question.

I typically use it to "Name" my HOC once I'm done coding it. So I'd typically use it at the top of a composed set of HOCs.

const withCoolFeatures = compose(
  setWrappedDisplayName('withCoolFeatures'),
  mapProps(),
  lifecycle(),
  connect(),
  whatever,
)(Foo);

I'd want to see a displayName of <withCoolFeatures(Foo)>. I don't necessarily want to see all the pieces that make up how withCoolFeatures works.

[jcheroske]

I just wanted to chime in and say that I, for one, like the idea of adding more helpers to recompose. I'm currently learning rxjs, and it has over a hundred operators I think. Many are simple derivations of others, analogous to the mapProps -> omitProps situation in this lib. I love that rxjs just gives you all the helpers. Just imagine how many omitProps implementations are floating around out there. Yikes!

[wuct]

@timkindberg I like the idea, but I don't think there is a way to achieve it without modifying other HOCs in recompose.

[hardchor]

From what I can tell, the issue is that HOCs like https://github.com/acdlite/recompose/blob/master/src/packages/recompose/withProps.js don't pass through the WrappedComponent (as is usually the case in, e.g. redux), so you won't have access to the original displayName. Is this something we could add?

Another use case (i.e. mine 😉) where having access to the WrappedComponent would be jest snapshot testing with shallow-rendered components.

[hardchor]

Bit awkward right now, but this is the solution (based on @timkindberg 's HOC) I came up with:

const setWrappedComponent = BaseComponent => setStatic('WrappedComponent', BaseComponent)(BaseComponent);
const setWrappedDisplayName = name => BaseComponent =>
  setDisplayName(wrapDisplayName(BaseComponent.WrappedComponent || BaseComponent, name))(
    BaseComponent,
  );

export default () =>
  compose(
    setWrappedDisplayName('toggleable'),
    // other HOCs
    hoistStatics(withState('toggled', 'setToggled', false)),
    hoistStatics(
      withHandlers({
        toggle: ({ setToggled }) => () => setToggled(n => !n),
      }),
    ),
    // ... and finally
    setWrappedComponent,
  );

[timkindberg]

@hardchor cool! You could totally make that into a reusable function. Call it namedCompose('toggleable')(...hocs). It would always setWrappedDisplayName at the beginning and setWrappedComponent at the end. All the intermediate HOCs passed in would be auto wrapped in hoistStatics.

[Isaddo]

@timkindberg @hardchor I think namedCompose could be implemented like this

const namedCompose = wrapperName => (...hocs) => BaseComponent =>
  setDisplayName(
    wrapDisplayName(BaseComponent, wrapperName) // build wrapped name
  )(
    compose(...hocs)(BaseComponent) // build enhaced component
  )

and use like this

export default namedCompose('toggleable')(
  withState('toggled', 'setToggled', false),
  withHandlers({
    toggle: ({ setToggled }) => () => setToggled(n => !n),
  }),
  // ... 
)

@wuct for naming intermediate, I tried this

const wrapName = wrapperName => ({
  $$composeWithIntermediateNaming: true,
  name: 'wrapperName',
})
const composeWithIntermediateNaming = (...hocs) => BaseComponent => compose(
  ...hocs.map(
    hoc => hoc.$$composeWithIntermediateNaming ?
      setDisplayName(wrapDisplayName(BaseComponent, hoc.name)) :
      hoc
  )
)(BaseComponent)

and the usage would like below

const enhance = composeWithIntermediateNaming(
  mapProps(/* do something */),
  wrapName('customizedName'),
  mapProps(/* do something */),
)(Foo)

but I would like use namedCompose instead

const enhance = compose(
  mapProps(/* do something */),
  namedCompose('customizedName')(
    mapProps(/* do something */),
  ),
)(Foo)

[timkindberg]

Or compose usage could be overloaded.

If a string is passed in as the only arg, then it returns a 'named' compose function.

compose('MyName')(...hocs)(BaseComponent);

Otherwise it works as previous:

compose(...hocs)(BaseComponent);

[ghost]

@timkindberg Not a big fan - that'd completely change the function signature.
I prefer the previous approach.

[timkindberg]

@burkhardr it would be backwards compatible. So the signature would break and we'd avoid YAHOC (Yet another higher order component).

[istarkov]

Ill close this. Feel free to reopen if you think that this discussion is not the road to nowhere