javivelasco / react-css-themr

Easy theming and composition for CSS Modules.
MIT License
591 stars 68 forks source link

Receiving/passing ref #65

Open klimashkin opened 7 years ago

klimashkin commented 7 years ago

Hi! To continue our conversation at https://github.com/javivelasco/react-css-themr/pull/46

In 2.1.0 you added mapThemrProps with example:

function mapThemrProps(props, theme) {
  const { composeTheme, innerRef, mapThemrProps, themeNamespace, ...rest } = props;
  return { ...rest, withRef: innerRef, theme };
}

But withRef is usually boolean, for example in react-redux, so it doesn't work.

It was case, when @themr hoc wraps another hoc. But what if vice-versa, I have places where it's wrapped by @connect.

It used to be simple convention: In hoc's ref handler function it checked if underline instance also has getWrappedInstance and used it to take ref from it, so it's a chain when you can reach real component no matter how many hocs do you have on top of it

    saveComponentRef(instance) {
      this.instance = instance && instance.getWrappedInstance ? instance.getWrappedInstance() : instance;
    }

But now having somewhere withRef, somewhere innerRef or mapThemrProps I'm confused. Feels like it has added complexity and problems instead of solving anything

klimashkin commented 7 years ago

Moreover, if I have hundred components with themr and redux, I need to write everywhere the same thing

function mapThemrProps(props, theme) {
  const { composeTheme, innerRef, mapThemrProps, themeNamespace, ...rest } = props;
  return { ...rest, withRef: innerRef, theme };
}

That adds absolutely redundant data transformation on each render just to get underlying ref.

raveclassic commented 7 years ago

@klimashkin Unfortunately I don't have enough practice with innerRef/withRef because, well, I just don't use it. But anyway, why can't we just pass actual ref prop through all the hocs? Does React somehow violate accessing it? I mean what was the point of entrodusing withRef/innerRef prop? Maybe @javivelasco can shed some light on it?

On the other hand, when you assume that underline instance has a getWrappedInstance method you rely on inner implementation, don't you?

Still after looking into the source there's a prop innerRef which accepts a function that is passed as an actual ref prop to decorated component. Can't you use it to pass the ref?

klimashkin commented 7 years ago

@raveclassic

why can't we just pass actual ref prop through all the hocs?

Because of React design - child component never receive it, react uses it to pass component instance of child element up to parent.

On the other hand, when you assume that underline instance has a getWrappedInstance method you rely on inner implementation, don't you?

And with innerRef the same - parent should know that child component wrapped in hoc with innerRef, but statically - on react element creation time. In case of getWrappedInstance we can check if child component is a hoc or not dynamically, inside save ref handler. That's why it's very handy for another hocs, they simply can't know about it statically without introducing new option like passInnerRef that I described here https://github.com/javivelasco/react-css-themr/pull/46#issuecomment-284483101

Still after looking into the source there's a prop innerRef which accepts a function that is passed as an actual ref prop to decorated component. Can't you use it to pass the ref?

Read about ref and this thread one more time : )

raveclassic commented 7 years ago

@klimashkin I see. Do you suggest reverting replacement of withRef with innerRef or do you have some more elegant solution?

klimashkin commented 7 years ago

That's why I asked @javivelasco at https://github.com/javivelasco/react-css-themr/pull/46#issuecomment-284483101 about introducing to react community new convention with two properties: innerRef and passInnerRef.

I'm pretty happy with current withRef and getWrappedInstance convention - it does job well. And because of new innerRef and mapThemrProps I can't update react-css-themr to 2.x version in my project , because it breaks hoc structure in many components. And because I can't update it I'm getting react-props warnings now. So I'm stuck in the middle : )

raveclassic commented 7 years ago

I don't believe that introduction of a new convention looks like a solution but still I agree that innerRef could be reverted back to withRef at least to be similar to other existing hocs.

klimashkin commented 6 years ago

The new ref forwarding api is coming to react

https://github.com/bvaughn/rfcs/blob/ref-forwarding/text/0000-ref-forwarding.md image

https://reactjs.org/docs/forwarding-refs.html