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

feat(forwardRef): add forwardRefFactory #2844

Closed
wants to merge 5 commits into from
Closed

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented May 26, 2018

Replaces #2306.


React 16.3 gives the wonderful forwardRef API, this PR implements the forwardRefFactory that allows to use it.

Some ideas are taken from klimashkin/react-forwardref-utils.

<Ref innerRef={ref}>
<Component {...props} />
</Ref>
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This factory allows to use stateless components too 😺

if (prop === forwardRefSymbol) {
acc.ref = props[forwardRefSymbol]
return acc
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hack allows to don't map ref in the every component

@layershifter
Copy link
Member Author

@levithomason this is an initial pass, no tests, just PoC. Will be interesting to hear your thoughts.

@mihai-dinculescu
Copy link
Member

mihai-dinculescu commented May 30, 2018

Could add displayName, it's quite useful in Dev Tools.

export const forwardRefFactory = (Component) => {
const forwarder = forwardRef(forwardFunctionFactory(Component))

hoistStatics(forwarder, Component, { $$typeof: true, render: true })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we hoist { $$typeof: true, render: true } here? Also, the values are conflicting with the docblock on this function.

* @return {Object}
*/
export const isClassComponent = Component =>
typeof Component === 'function' && Component.prototype && !!Component.prototype.isReactComponent
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we also add:

export const isStatelessComponent = Component =>
  typeof Component === 'function' && Component.prototype && !Component.prototype.render

export const supportsRef = Component => 
  typeof Component === 'string' || isClassComponent(Component)

It may help clarify the factory function, see below.

// eslint-disable-next-line react/prop-types
if (_.isUndefined(props.as) || _.isString(props.as) || isClassComponent(props.as)) {
return <Component {...{ [forwardRefSymbol]: ref, ...props }} />
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of explicitly handling the ref for supported use cases and returning the Component in all other cases?:

import { isStatelessComponent, supportsRef } from './componentUtils.js'

export const forwardFunctionFactory = Component => (props, ref) => {
  /* eslint-disable react/prop-types */

  if (isStatelessComponent(props.as)) {
    return (
      <Ref innerRef={ref}>
        <Component {...props} />
      </Ref>
    )
  }

  if (supportsRef(props.as)) {
    return <Component {...{ [forwardRefSymbol]: ref, ...props }} />
  }

  return Component
}

@levithomason
Copy link
Member

This implementation is far better, thanks!

Also, this pattern should definitely be implemented in the v2 fork. I'm about to post a set of specification proposals for the lib directory. This directory is becoming a core lib called "Stardust". Our library and others will be built on the standards there. Dealing with refs is one of those standards.

@layershifter layershifter changed the base branch from react-16 to master July 20, 2018 13:23
@layershifter layershifter changed the title feat(forwardRef): add forwardRefFactory BREAKING(forwardRef): add forwardRefFactory Jul 20, 2018
@layershifter layershifter changed the title BREAKING(forwardRef): add forwardRefFactory feat(forwardRef): add forwardRefFactory Jul 25, 2018
@layershifter layershifter changed the base branch from master to react-16.3 July 25, 2018 09:01
@layershifter layershifter changed the base branch from react-16.3 to master November 7, 2018 15:26
layershifter and others added 3 commits November 7, 2018 17:28
Signed-off-by: Oleksandr Fediashov <ofediashov@exadel.com>
Signed-off-by: Oleksandr Fediashov <ofediashov@exadel.com>
@layershifter
Copy link
Member Author

We decided to stop our work there, microsoft/fluent-ui-react#587 (comment).

There is a RFC reactjs/rfcs#97 that possibly will simplify all.

@layershifter layershifter deleted the feat/forward-ref branch December 14, 2018 09:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants