Popmotion / popmotion

Simple animation libraries for delightful user interfaces
https://popmotion.io
19.99k stars 662 forks source link

React Pose: React warning about innerRef #473

Closed inomdzhon closed 5 years ago

inomdzhon commented 6 years ago

Describe the bug

When converted existing components with spread props to posed components by calling posed using posed React throw warning:

Warning: React does not recognize the `innerRef` prop on a DOM element. If you intentionally want it to appear in the DOM as a custom attribute, spell it as lowercase `innerref` instead. If you accidentally passed it from a parent component, remove it from the DOM element.

How to reproduce

Steps to reproduce the behavior:

  1. Create component with provide rest props (const { ...rest } = props; <div {...rest} />)
  2. Converted to posed components by calling posed posed(...)({})
  3. Open Dev Tools console
  4. See warning

Expected behaviour

No warning

Link to CodePen example (or similar)

https://codesandbox.io/s/m3mwz6w2jx

Andarist commented 6 years ago

https://github.com/Popmotion/popmotion/blob/9a2a50b448a9debf2457bea604b3b19b0a54421d/packages/react-pose/src/components/PoseElement.tsx#L174-L179

You should also unpack innerRef, like this:

function Box(props) {
  const { className, style, hostRef, innerRef, ...rest } = props;
  return <div className={className} style={style} ref={hostRef} {...rest} />;
}

Seems to me that popmotion should decide on a single ref prop.

mattgperry commented 6 years ago

There was some reason we needed innerRef as well, to do with Styled Components. BUt I think they're moving to ref now so I might take another look soon

On Mon, 10 Sep 2018 at 13:23 Mateusz Burzyński notifications@github.com wrote:

https://github.com/Popmotion/popmotion/blob/9a2a50b448a9debf2457bea604b3b19b0a54421d/packages/react-pose/src/components/PoseElement.tsx#L174-L179

You should also unpack innerRef, like this:

function Box(props) { const { className, style, hostRef, innerRef, ...rest } = props; return <div className={className} style={style} ref={hostRef} {...rest} />; }

Seems to me that popmotion should decide on a single ref prop.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Popmotion/popmotion/issues/473#issuecomment-419893758, or mute the thread https://github.com/notifications/unsubscribe-auth/AHfLKs9m76QEr1-mNR4ZX5U_TxM91PcJks5uZlm5gaJpZM4WhQV4 .

inomdzhon commented 6 years ago

@Andarist Yeah, I have yet to do so. Sure, may be leave only innerRef props sound good, because many library and some people use this naming.

Andarist commented 6 years ago

@InventingWithMonster u wont be able to integrate with styled-components seamlessly, they are migrating to ref on styled components themselves, so if u know that a component is a styled component then u can use a ref instead of innerRef. But u have to be able to wrap any component with posed and u wont be able to distinguish components which are using forwardRef (such as styled components) from others, so u have to innerRef (or similar) for any composite component that u receive, its unfortunately a user’s responsibility to tie those 2 together. The problem here at hand seems to be that u are not opinionated enough and u are providing 2 ref props to the wrapped component. I would advise to stick to one of those names

Andarist commented 6 years ago

Also this got me thinking - styled-components (and other libraries) migrating to forwardRef are making in fact the interop between "wrapping" libraries harder.

~~From what I see in styled-components codebase https://github.com/styled-components/styled-components/blob/d21448d9898be6b51a45e07a3c428284a3ae74af/src/models/StyledComponent.js#L123 you would have to use now forwardedRef for a better interop as styled-components will use that as ref.~~

~~EDIT:// actually that won't work as forwardedRef is their internal prop created here https://github.com/styled-components/styled-components/blob/d21448d9898be6b51a45e07a3c428284a3ae74af/src/models/StyledComponent.js#L257~~

EDIT2:// I've actually found a way to support this nicely for upcoming styled-components release (and other libraries using forwardRef API. The PR is here.

Either way I would still argue that it would be desirable to either:

inomdzhon commented 6 years ago

Hi, again)

I found nice solution for resolve Typescript Error and Pose Error – using innerRef instead hostRef.

// No error: Uncaught Error: HEY, LISTEN! No DOM ref found.
function Box(props) {
  const { className, style, innerRef, ...rest } = props;
  return <div className={className} style={style} ref={innerRef} {...rest} />;
}

// No error: Property 'hostRef' is missing type
const TweenBox = posed(Box)({});
DevanB commented 5 years ago

Was a solution found for this? I just tried to use react-pose with styled-components v4 and can't seem to find a working solution. Thanks!

Andarist commented 5 years ago

https://github.com/Popmotion/popmotion/blob/09b5fad9a1cd8ff56074d79268e0f23ad4c66aa1/packages/react-pose/src/components/PoseElement/index.tsx#L181

I bet Matt would appreciate help with this one to fix your issue.

DevanB commented 5 years ago

@Andarist I would love to help fix the issue...if I understood the issue more. I've barely got a grasp on refs, much less hostRef and innerRef.

hostRefs, innerRefs, oh my!

Andarist commented 5 years ago

Dont worry - fix is on the way #580

florianbepunkt commented 5 years ago

@Andarist Could you please give an example how to use Pose with StyledComponents? I read the blog post, but still get the following error

Error: HEY, LISTEN! No valid DOM ref found. If you're converting an existing component via posed(Component), you must ensure you're passing the ref to the host DOM node via the React.forwardRef function.

this is my styled component wrapped with the forwardRef method

import { Box } from 'grommet';
import React from 'react';
import styled from 'styled-components';
import { forwardRef } from 'react';

const FabStyledBox = styled(Box)`
  &&& {
    box-shadow: 0px 3px 8px rgba(100, 100, 100, 0.5);
    height: 56px;
    width: 56px;
    z-index: 100;
  }
`;

class FabButton extends React.PureComponent {
  render() {
    return (
      <div ref={this.props.ref}>
        <FabStyledBox>{label}</FabStyledBox>
      </div>
    );
  }
}

export default forwardRef((props, innerRef) => (
  <FabButton {...props} ref={innerRef} />
));

and then I import this FabButton and use it like so

const PosedFabButton = posed(FabButton)({
  enter: { opacity: 1, delay: 1000, transition: { duration: 1000 } },
  exit: { opacity: 0, delay: 1000, transition: { duration: 1000 } }
});

<PosedFabButton
  key="FAB"
  label="Partner hinzufügen"
/>
Andarist commented 5 years ago

With styled-components@4 you dont have to do anything special with forwardRef if you wrap styled component with posed directly - https://codesandbox.io/s/o47r744p25