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

Suggestion for using forwardRef for imperative API (QUESTION) #56

Closed
oriSomething opened this issue Jul 30, 2018 · 4 comments
Closed

Suggestion for using forwardRef for imperative API (QUESTION) #56

oriSomething opened this issue Jul 30, 2018 · 4 comments

Comments

@oriSomething
Copy link

I'm not sure it's the right place but it's not a bug so I'm not sure where is the right place. (Maybe a new API)

In project I work on, there is a TextField component which is more than a wrapper for <input>. It wraps the <input> element with a <div> to overcome some limitation that needed by a requested design.

TL;DR

The forwardRef API as is seems to not be usable for this use case, because:

  1. The actual wrapper is a <div> which is not what you expect when you manually want the textfield element. Because you would expect the <input> for using commands such as #focus() or #checkValidity() and so

  2. But sometimes you do need the <div> container because you need to measure the element which the inner <input> is useless for. ReactDOM.findDOMNode from what I've search isn't recommended. But it also blocks the ability to use the code in React Native or other renderers

Today, I add functions to the component class such as TextField#focus() (that calls focus on the <input>) and I'll probably need to add one for TextField#getClientRects() (for measure the <div>). But, it seams redundant and not very stable as more native DOM functionality is needed to warped by the component class for imperative needs use cases (as the project grows and more needs are added).

What do you think I should do?
Maybe forwardRef need some tweaks?
Maybe a new API is needed?
Or maybe the current scenario of what I do is the least worst option?

@j-f1
Copy link

j-f1 commented Jul 30, 2018

You could set up your forwardRef to target the actual <input>, then add a wrapperRef or containerRef prop that you can then pass to the <div>, allowing people to choose which element to reference.

@oriSomething
Copy link
Author

@j-f1 First of all, thanks for the response. But than it wouldn't be straight forward to use in cases when ref comes from a "render prop" pattern. For example <Tooltip {…} renderTooltip={…} render={(props) => <TextField {...props} />} />.

@j-f1
Copy link

j-f1 commented Jul 30, 2018

If you need the <div> ref more often, you could make the ref prop connect to the <div> and have a inputRef param that connects to the <input>.

@sag1v
Copy link

sag1v commented Oct 14, 2018

I would like to add an example to be sure i'm not missing something here.
Sometimes we want to get the DOM node without caring what type of children we get (function, text, class or regular DOM element).

findDOMNode was the perfect tool for this task.

Given this component:

class Trap extends React.Component {
  state = {
    trapped: false
  };

  componentDidMount() {
    const { event } = this.props;
    this.ref = findDOMNode(this);
    document.addEventListener(event, this.handleEvent);
  }

  componentWillUnmount() {
    const { event } = this.props;
    document.removeEventListener(event, this.handleEvent);
  }

  handleEvent = ({ target }) => {
    if (this.ref && this.ref.contains) {
      const trapped = this.ref.contains(target);
      this.setState({ trapped });
    }
  };

  render() {
    const { children } = this.props;
    const { trapped } = this.state;

    if (typeof children === "function") {
      return children(trapped);
    } else {
      return null;
    }
  }
}

We could use it with a simple API:
<Trap event="click">{trapped => <Box isFocused={trapped} />}</Trap>

But now with out findDOMNode we invert the responsibility of handling the ref to the consumer.
So our API must change and expose the ref function for the consumer to use it:

class TrapWithRef extends React.Component {
  state = {
    trapped: false
  };

  componentDidMount() {
    const { event } = this.props;
    document.addEventListener(event, this.handleEvent);
  }

  componentWillUnmount() {
    const { event } = this.props;
    document.removeEventListener(event, this.handleEvent);
  }

  handleEvent = ({ target }) => {
    if (this.ref && this.ref.contains) {
      const trapped = this.ref.contains(target);
      this.setState({ trapped });
    } else {
      console.log("no ref -> ", this.ref);
    }
  };

  render() {
    const { children } = this.props;
    const { trapped } = this.state;

    if (typeof children === "function") {
      return children(trapped, ref => (this.ref = ref));
    } else {
      return null;
    }
  }
}

The consumer will use it this way:

<TrapWithRef event="click">
  {(trapped, ref) => <Box innerRef={ref} isFocused={trapped} />}
</TrapWithRef>

Of course this is limiting as the consumer can't "just" render anything. for example text is out of the question, but more important the consumer will need to pass the ref down to <Box /> and attach it somewhere (let's say box is not a class component).

I don't mind this change as i understand this is a minor downgrade compare to the benefits we gain with ditching findDOMNode. I just want to be sure I'm not missing anything or maybe there is a way to keep this behavior without findDOMNode .

@gaearon gaearon closed this as completed Aug 24, 2021
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

4 participants