Recompose: Best practice on recompose added props

[nfantone]

Hi! First, I'd like to apologize for opening an issue for this: might not be the proper place. If it's not, feel free to close it.

I've always liked recompose and have used it several times in some personal, rather small, projects of mine - but never in a full scale, production-ready app. A couple of months ago, we started a new middle-sized project over at work and, after some discussion, we bit the bullet and decided to include recompose

The app is now several components large (order of dozens) and the experience has been good, so far. However, the thing we all agreed on as being the least likeable side-effect of our decision is the emergence of what we call _"private props"_: props not explicitly declared by PropTypes (or flow types), not part of a component's API, but expected as arguments. Here's an example of such scenario:

const propTypes = {
  onFilter: PropTypes.func.isRequired
};

const CaseFilterForm = ({
  inputs,
  addInputRefs,
  filter,
  onReset,
  onSubmit,
  onInputChange
}) => (
  <form onSubmit={onSubmit} className="filter">
    <fieldset>
       { ... }
    </fieldset>
  </form>
);

export default compose(
  setDisplayName('CaseFilterForm'),
  setPropTypes(propTypes),
  withProps({ inputs, inputRefs: [] }),
  withState('filter', 'setFilter', {}),
  withHandlers({ ... })
)(CaseFilterForm);

You use this component like so:

<CaseFilterForm onFilter={filters => {...} } />

As shown above ☝️ , it's very common for _"private props"_ to outnumber "common"/API props - and even not being received by the actual component signature, at all (onFilter is used inside one of the handlers defined in withHandlers). This leads to a situation were it's not immediately clear what properties are expected by the component, hinders development experience (specially for newcomers) and makes for less readable code overall, having to jump back and forth from the component's declaration to the composed HOCs section to get a grasp of what's going on. All things that we were hoping to actually improve by leveraging recompose tools.

So, my question here is: is there any best practice or recommendation to handle cases like these? Any good rule of thumb? Any feedback would be appreciated! Thanks.

[istarkov]

I'm not sure I understand the problem, but partially I think I understand.
1) Naming - name private handlers and props for example with underscore at the end. Or use any other name schema. (I historically use handle prefix for private handlers and on for public) And underscore at the end for private data props (sometimes)
2) Namespacing - group internal props somehow
3) For some heavy components render props patterns allows sometimes to group and visually split some props.

({ onFilter, blaBla }) => <MyComponentWithHocs onFIlter={onFilter}>{
  ({ someProps, someOtherProps }) => <OtherComp {...{onFilter, someProps, someOtherProps }}>
}

useful link https://github.com/jaredpalmer/awesome-react-render-props

[istarkov]

4) For reusable components I use and create storybooks with multiple usage examples, storybooks are good playground and docs.

[WaldoJeffers]

I have the same issue: if I don't declare propTypes matching my "private" properties , ESLint will warn me that I need to declare them, which leads to adding a big number of propTypes. On the contrary, if one of my "real" properties is used only internally (by a "private" property), ESLint will warn me that I have a unused propType (which is wrong). What's the best practice on this?

[istarkov]

disable stupid eslint rules

[nfantone]

Well, that depends on what we understand by "stupid". Maybe those rules are useful somewhere else or are used to keep consistency across projects.

BTW, those are all very good suggestions @istarkov. I feel they are closer to general, good practice applications rather than something recompose/React specific, though.

[GollyJer]

@nfantone Not sure if this is helpful but thought I'd chime in.
We namespace our _"private props"_ and group them by functionality.
A simple example can be found here: Is it possible to wrap event handlers in a parent object?

With this approach refactoring and getting up to speed on a component has been a breeze. Items prefixed in my are always "private", all others are part of the API.

[nfantone]

@GollyJer Interesting idea. I don't dislike the namespacing approach.

[joekrill]

Sorry if I'm late to the party on this, and I hope I'm not misunderstanding the problem here, but why not declare both sets of props? This is what we've been doing:

const CaseFilterForm = ({
  inputs,
  addInputRefs,
  filter,
  onReset,
  onSubmit,
  onInputChange
}) => (
  <form onSubmit={onSubmit} className="filter">
    <fieldset>
       { ... }
    </fieldset>
  </form>
);

CaseFilterForm.propTypes = {
  inputs: PropTypes.arrayOf(...),
  addInputRefs: PropTypes.arrayOf(...),
  filter: PropTypes.shape({ ... }),
  onReset: PropTypes.func.isRequired,
  onSubmit: PropTypes.func.isRequired,
  onInputChange: PropTypes.func.isRequired,
};

export default compose(
  setDisplayName('CaseFilterForm(enhanced)'),
  setPropTypes({
    onFilter: PropTypes.func.isRequired,
  }),
  withProps({ inputs, inputRefs: [] }),
  withState('filter', 'setFilter', {}),
  withHandlers({ ... })
)(CaseFilterForm);

In the case of props being passed through, you can do things like:

  setPropTypes({
    onSubmit: CaseFilterForm.propTypes.onSubmit,
  }),

All of this:

1. Makes it clear that there are two distinct components (CaseFilterForm and CaseFilterForm(enhanced)), and makes it explicitly clear what props each component is expecting.
2. Catches errors if someone changes the "enhanced" component in an incorrect way (via propType warnings).
3. Satisfies eslint

I guess some folks might complain that this may be overkill? But to me that's akin to arguing that comments aren't necessary, and I'm not going to go down that road (we also use jsdocs on our propType declaration). I prefer this level of documentation, personally.

[wuct]

@joekrill I’ve done same thing before and I see no problem with this approach.

[nfantone]

@joekrill Well, but that's just the thing I was concerned about. Most of the props you declared on CaseFilterForm can't actually be set from the outside (given that we are not exporting the component but its "enhanced" version). So things like these won't work:

<CaseFilterForm inputs={[ ... ]} filter={ { .. } } />

They are not _really_ part of your component's API, even though they are _declared in your PropTypes_. Problem also goes the other way: you are receiving some props that are not specified in your PropTypes (like setFilter). I believe the problem arises from the fact that we are internally working with two components, while exposing only _one_ using a single name. Perhaps the definitive solution would be to split them in two files? If you were tempted to call your alternative an "overkill", I don't doubt we can agree that that would definitely be it.

Bottom line being: while your proposal does make things a bit clearer, a developer that wants to use your component cannot easily assert which props it expects (keyword here being: "_easily_"). That's the problem that I personally see with the approach, @wuct.

[joekrill]

@nfantone Sorry but I really just don't understand your concern. Am I missing something? What does it matter whether they can be set from the "outside" or not? Just because something isn't part of your "public" API doesn't mean that proptype declarations aren't useful. They still give you error/sanity checking. They validate that your enhanced component is providing the appropriate props to your base component. They document your code. They are extremely useful whether they are part of your component's public API or not.

I use jsdocs/esdocs quite a bit. I document both "private" functions as well as "public" functions. They are useful regardless of whether the function is deemed to be "public". PropTypes are providing similar information here (plus additional benefits). I just don't see any difference.

Bottom line being: while your proposal does make things a bit clearer, a developer that wants to use your component cannot easily assert which props it expects (keyword here being: "easily"). That's the problem that I personally see with the approach, @wuct.

How so? It's _extremely_ clear. There is one very distinct set of propTypes defined for your "private" component, and a separate and distinct set of propTypes for your "public" enhanced component.

Will 2 separate files make it even clearer? Possibly. That's more of a personal/stylistic preference for an individual project. I could absolutely see an argument for having a CaseFilterForm.js that houses the base component, and a separate CaseFilterFormEnhanced.js that houses the enhanced component. I'd still define all the prop types on each version of the component, though.

[wuct]

I think we are on the same page. As @nfantone said, what we need is two sets of PropTypes definition because there are two components: the original one and the enhanced one. And, I agree with @joekrill that separating them into two files is more of a personal reference.

[nfantone]

@wuct here has summarized this perfectly. And yes, I think we are on the same page - even if we _are_ reading different paragraphs.

Nevertheless, let me address your remarks @joekrill:

@nfantone Sorry but I really just don't understand your concern. Am I missing something? What does it matter whether they can be set from the "outside" or not?

It matters to developers trying to read and use your code. Take a React component from the wild. Almost any one will do. You have a (mostly) clear set of propTypes (or some flavour or it) that reveals _expected props_ and document the API. It answers the question "How do I use this alert/date picker/badge?" in a deterministic, reliable manner. Using solutions like recompose (even with your proposed practice) _obscures_ that answer.

Just because something isn't part of your "public" API doesn't mean that proptype declarations aren't useful. They still give you error/sanity checking. They validate that your enhanced component is providing the appropriate props to your base component. They document your code. They are extremely useful whether they are part of your component's public API or not.

Nobody is arguing the benefits of documenting your code, be it via prop types, jsdocs or any other form. It is undeniably useful and the reasons should be (are?) self-evident. My original question was about "best practices" on revealing those reasons to others while using composition with React.

It certainly doesn't help that even the official documentation uses PropTypes to typecheck props that are explicitly part of a component's API in _all of their examples_.

How so? It's _extremely_ clear. There is one very distinct set of propTypes defined for your "private" component, and a separate and distinct set of propTypes for your "public" enhanced component.

That's a _very_ personal opinion. I totally understand where you're coming from and, while it may be clear to you (and your team), I can confidently tell you that it is not for many people. Specially new comers or people not familiar with functional composition. I can imagine questions close to the following would be common:

• "Why are there two sets of PropTypes definitions here?"
• "Which one should I use?"
• "Why is this component rendering X value when I'm explicitly passing Y to it as prop?"

Will 2 separate files make it even clearer? Possibly. That's more of a personal/stylistic preference for an individual project. I could absolutely see an argument for having a CaseFilterForm.js that houses the base component, and a separate CaseFilterFormEnhanced.js that houses the enhanced component.

100% agree. Specially if we include unit testing talk in that hypothetical argument.

[istarkov]

guys just develop, you'll find a good patterns for u and for your team very fast. Imo no patterns exists. All the code I wrote just 1 y. ago looks like a shit ;-) Just code, thats enough :-)

[soulclown]

Hi all, very cool intervensions and still actual topic!
As @nfantone said :

It matters to developers trying to read and use your code. Take a React component from the wild. Almost any one will do. You have a (mostly) clear set of propTypes (or some flavour or it) that reveals expected props and document the API. It answers the question "How do I use this alert/date picker/badge?" in a deterministic, reliable manner. Using solutions like recompose (even with your proposed practice) obscures that answer.

That is something I do not want to underestimate, regardless of what decision we make (split component in more parts or not , maintain a different prototype for enhancer or not , doubling tests or not ).

I want to share some thoughts about my team experience using RecomposeJS for 6 months in production:

• improvements in performance ends raising up code complexity ;
• developers tends unintentionally to add logic in stateless components ;
• (consequently) we are far away from the initial idea behind components compositions.

Somebody already experienced what i'm talking about?
Thanks and keep coding !

[sallyhill]

Maybe having a secondary argument on the HOCs for static properties would be good?