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

Offer an opinionated helper for HOC creation for authors #9345

Closed
Pomax opened this issue Apr 5, 2017 · 5 comments
Closed

Offer an opinionated helper for HOC creation for authors #9345

Pomax opened this issue Apr 5, 2017 · 5 comments

Comments

@Pomax
Copy link

Pomax commented Apr 5, 2017

HOCs are great, I think we can all agree. What is not so great, however, is that React has no opinion on how people should implement HOCs, and so both users and authors alike have no guideline when it comes to implementing a consistent api for "using multiple HOCs", especially when those HOCs get chained. As such, I think that there's an opportunity to make the lives of both developers who work with React, as well as HOC maintainers who have to make sure their HOCs work in a variety of code bases easier by having React be slightly opinionated on how to write a HOC.

To describe the problem: right now there is no established way to ask a HOC for the component class(es) that were put in, nor an established way to access the "real element" that a HOC wraps. As such, each HOC has to either settle for "too bad for my users" or invent its own API for this, and as many have gone the second route, there are a fair few different ways that this access has been implemented over different HOCs.

To fix this, I'd like to suggest establishing a best practice, guided by React, by adding a React.createHOC({ ... }) mechanism with corresponding import HOC from React; class Thing extends HOC { ... }" to the next version of React, so that users and authors no longer need to guess APIs.

My suggestion would be for this React.HOC class to define:

  • a constructor that takes an arbitrary number of Component classes
  • a getClasses() function that returns the (list of) Component class(es) that were passed in
  • a getInstances() function that returns the (list of) element(s) that the HOC actually builds (and knows how to access by ref based on the Component class names)

As an example (and the following code is public domain - which I unfortunately need to be point out because I am unable to sign the CLA necessary to file PRs, so I can't file a PR in conjunction to this issue. As such, the following code lacks as much implementation as possible so as to allow for PRs by others):

class HOC extends Component {
  constructor(...componentClasses) {
    this.componentClasses = componentClasses
  }

  getClasses() {
  /*
    Passed component might themselves be HOCs, so this function needs
    to perform an iteration check to build a valid list of contained classes.

      classesList = this.componentClasses.map( componentClass => {
        if (componentClass.getClasses) {
          return componentClass.getClasses()
        }
      }).filter( result => result)

    probably paired with an array flattening operation
  */
  }

  getInstances() {
  /*
    the same holds here; in order to make this work, a naming convention needs
    to be declared around refs: if you use React.HOC then you will name your refs
    after the component class you pushed in.

      instanceRefs = this.componentClasses.map( c => c.constructor.name )
      elements = instanceRefs.map(ref => this.refs[ref])

    this then needs the same check before we can return it:

      elements = elements.map(e=> e.getInstances ? e.getInstances() : e)

    and again probably with an array unpack so it's a flat list.
  */
  }
}

This lets React solve the problem of standardizing the barebones functionality that users and authors should be able to rely on to "just work", without interfering with how HOCs currently work, or dictating how people should write their HOCs: you can keep writing HOCs as plain Components that wrap one or more components, but extending from HOC now offers you a zero-effort option to impart some functionality for free that users will appreciate.

There is an interesting suggestion in jsx-eslint/eslint-plugin-react#678 (comment) that instead relies on rebinding props, but this might not good enough as this would also expose refs used by the HOC for internal purposes, and the user should not be able to fiddle with.

Anyway, this is mostly an issue filed to get a ball rolling on not so much standardizing "how HOCs should be made" (it feels like React should stay unopinionated on that) but on having React offer a helping opinion on one way in which you can write a HOC, with the promise that using that means you're now compatible with any other HOC that uses the same suggestion.

@aweary
Copy link
Contributor

aweary commented Apr 5, 2017

Hey @Pomax, for taking the time to write this up!

HOCs are great, I think we can all agree. What is not so great, however, is that React has no opinion on how people should implement HOCs, and so both users and authors alike have no guideline when it comes to implementing a consistent api for "using multiple HOCs", especially when those HOCs get chained.

There is a doc page on higher-order components in the official React docs. Do you have any suggestions on what we could add to make it clearer or more robust? Better documentation and discussions focusing on best practices might be better than expanding the public API.

To describe the problem: right now there is no established way to ask a HOC for the component class(es) that were put in, nor an established way to access the "real element" that a HOC wraps.

Isn't a HOC typically a function that takes a single component class and returns another composing it? Can you expand on HOCs where multiple classes are passed in? Are you referring to utilities like compose that take multiple HOCs and compose them with a component class?

I think offering a class-based HOC solution would probably be moving in the wrong direction, as existing HOCs are typically just functions and are composed using functional patterns.

in order to make this work, a naming convention needs to be declared around refs: if you use React.HOC then you will name your refs after the component class you pushed in.

Standardizing access to elements by recommended a ref naming pattern isn't likely to work either since string refs are a legacy API. @gaearon makes some good points on that problem in the thread you linked as well (jsx-eslint/eslint-plugin-react#678 (comment))

@acdlite you've worked with HOCs extensively with recompose and others, I'd love to hear your thoughts.

@Pomax
Copy link
Author

Pomax commented Apr 5, 2017

Mostly I'm trying to think of a good solution where the HOC functions all return an object with same API for accessing the "actual content wrapped" even if they share nothing else. I will admit: I've not seen any HOCs that take more than one component class as input in the wild, but I'm sure I've not seem most of them, and there is technically nothing in the docs or the code that prevent you from writing a HOC like this:

export default function wrapComponent(Thing, SpecificThingController, options) {
  ...
  return (props) => {
    <div>
      <Thing {...props} {...options} />
      <SpecificThingController {...props} {...options} />
    </div>
  } 
  ...
}

I admit, writing that feels super weird, but React will happily run with it if you write your HOC that way. If HOCs "should" only wrap a single component then that could be a thing to point out over on https://facebook.github.io/react/docs/higher-order-components.html, but the main thing I'm looking for is the docs being an authority and saying:

"There are a million ways to add functionality to your HOC so that your users can get to the original Component class and element-as-rendered, but if you do it this way, then your code will be able to interoperate with other HOCs that follow this suggested pattern, too"

which in itself is of course only half a step away from then following up with:

"If you don't want to copy-paste this code all the time, then for purely functional HOC <run the code through a React.helper function or something?> and for HOC that return a custom component you can make your component extend React.HOC instead of React.Component, or use React.createHOC rather than React.createClass"

(those might be misnomers, I guess technically those would be React.HOCClass and React.createHOCClass but I will the first to admit those are super inconvenient to read and write!)

For purely functional HOCs, you're right in that this solution falls short - I will have to think about that aspect a bit harder (my own HOC composes a full class as it needs to hook into lifecycle methods itself, so that made me come at this from a fully qualified Component perspective). I'd love to hear thoughts on how to ensure that there is a way to get to the original Component's class/element in a HOC chain both for fully qualified component HOCs and purely functional HOCs though. It's a problem that seems to come up time and again but is never quite big enough to boil over into a "we really need to fix this" so everyone's rolling their own solutions and we can probably come up with something better =)

@Andarist
Copy link
Contributor

Andarist commented Apr 5, 2017

Are you referring to utilities like compose that take multiple HOCs and compose them with a component class?

That is my original problem described in the linked comment in the thread starter post. There is no standard way for A and B geting access to C's ref in compose(A, B)(C) scenario.

I think offering a class-based HOC solution would probably be moving in the wrong direction, as existing HOCs are typically just functions and are composed using functional patterns.

Agree with you, that it's moving things in the other direction than they are heading nowadays. From my perspective there is not much need for new APIs, more just the best practices documented. Kinda protocol that might be applied to make HOCs interoperable.

Standardizing access to elements by recommended a ref naming pattern isn't likely to work either since string refs are a legacy API.

Standard name doesnt have to contain string value. It would work with functions as well, but as HOC doesnt know when to stop its not possible for it not to leak a 'special' wrappedRef (example name) prop to the wrapped component, so it can call it directly, exposing its ref to the HOCs (which might not be the worst thing in the world, and probably would allow some extra patterns to emerge - like wrapped component finally calling this function with something else than itself). But that would enforce wrapped component to be aware that it is wrapped, which sounds not right.

@Andarist
Copy link
Contributor

@aweary @gaearon @acdlite any more thoughts on this? Would love to push this issue forward and include a documentation section.

@aweary
Copy link
Contributor

aweary commented Sep 20, 2017

Coming back to this, I don't think the solution proposed here of expanding the API to provide first-class HOC support is something that will be pursued. The core of the issue is that it's difficult to access refs of wrapped components. This issue is being worked on independently (see #10581, #4213 for example) and a solution to that problem will likely address the bulk of this one.

@aweary aweary closed this as completed Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants