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

forwardRef precludes use of composite component test utils methods #13455

Closed
noahgrant opened this issue Aug 22, 2018 · 19 comments
Closed

forwardRef precludes use of composite component test utils methods #13455

noahgrant opened this issue Aug 22, 2018 · 19 comments

Comments

@noahgrant
Copy link

Do you want to request a feature or report a bug?
Bug, I believe—requested to file a new issue per #12453 (comment)

What is the current behavior?
When using ReactTestUtils that navigate the trees for composite components, I am unable to find instances of components wrapped in React.forwardRef:

findRenderedComponentWithType(tree, myHOCForwardedComponent)
// error, finds 0 instances

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem:

JSFiddle link here

I have a HOC that returns a forwardRef pretty much exactly like the one written up in the docs, except while using React Context:

const MyContext = React.createContext(someDefault);

const withMyContext = (Component) => {
  class MyContextConsumer extends React.Component {
    render() {
      const {forwardedRef, ...rest} = this.props;

      return (
        <MyContext.Consumer>
          {(value) => (
            <Component
              {...rest}
              ref={forwardedRef}
              myValue={value}
            />
          )}
        </MyContext.Consumer>
      );
    }
  }

  return React.forwardRef((props, ref) => (
    <MyContextConsumer {...props} forwardedRef={ref} />
  ));
};

@withMyContext
class MyHOCForwardedComponent extends React.Component {
  render() {
     return <div>HELLO</div>;
  }
}

What is the expected behavior?
I would hope that we could still navigate the tree, such that

findRenderedComponentWithType(tree, myHOCForwardedComponent)

is able to find the rendered instance.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
React 16.4—affected everywhere, I believe.

Thank you for the time!!

@gaearon
Copy link
Collaborator

gaearon commented Aug 22, 2018

I think this works as expected.

The ForwardRef() component itself does not have an instance. It merely passes down the ref it receives as a second argument. But it doesn't know what's going to happen to that ref. In your example it's attached to <MyContextConsumer {...props} forwardedRef={ref} />, and that later gets attached to <Component ref={forwardedRef} />. But there's no way to know that. It could've been passed further below, to some completely different child. Since ForwardRef component itself doesn't have an instance, we don't return anything.

I understand how this can be annoying. The truth is that TestUtils traversal utilities aren't great in general. There's a better traversal API for the Test Renderer, but React DOM doesn't expose it. We might want to revisit this as part of #9505 later.

@gaearon gaearon closed this as completed Aug 22, 2018
@noahgrant
Copy link
Author

Thanks, @gaearon, I was afraid of that. To be honest, I find TestUtils utilities very useful for simply finding an instance of a component and asserting the props passed to it. I'm finding it very difficult with.forwardRef, and I think we may have to bypass using it altogether. Do you have a recommended approach for that? We use a mix of both Enzyme's shallow rendering and raw ReactDOM.render + TestUtils. Thank you!

@gaearon
Copy link
Collaborator

gaearon commented Aug 23, 2018

Have you considered keeping the original type attached as a property to the wrapped one? Then you can search for it in tests. I don’t see the difficulty.

@noahgrant
Copy link
Author

No, I didn't think of that, and that might work, but I'm not sure I completely understand what you mean. I try not to dive into React internals in our application code, but are you suggesting something like attaching instance._reactInternalFiber.type to a property on the object returned by .forwardRef that the TestUtils tree traversal methods will use to pick it up? Sorry if I should already see what you're referring to.

@noahgrant
Copy link
Author

I'm thinking something like:

const withMyContext = (Component) => {
  class MyContextConsumer extends React.Component {
    render() {
      // ... same 
    }
  }

  return {
    ...React.forwardRef((props, ref) => (
      <MyContextConsumer {...props} forwardedRef={ref} />
    )),
    type: Component
   };
};

and then I can call things like:

findRenderedComponentWithType(tree, myHOCForwardedComponent.type)

Is that what you were referring to? Thanks again for the time.

@gaearon
Copy link
Collaborator

gaearon commented Aug 23, 2018

I’m not sure spreading a function like this would work. I just mean

const Result = forwardRef(...)
Result.Original = Component
return Result

And then yes, search for the .Original type.

@noahgrant
Copy link
Author

Gotcha. I spread the function return, which should be an object; seems to work fine. Thanks for the tip!!

@gaearon
Copy link
Collaborator

gaearon commented Aug 23, 2018

Ahh right it's an object. Yeah.

@noahgrant
Copy link
Author

@gaearon the solution worked beautifully in development. Oddly, I deployed to production and in the production build only the .forwardRef caused some flakiness—most of the time it worked but in some instances of the component (reliably each time), it did not—in some cases I noticed that componentDidMount was being called in an infinite loop.

I've been debugging for a bit and haven't come up with anything concrete. I'll try to put together a fiddle in the next couple days. But it was really concerning to me that it worked so well in dev, and it was only in prod that I ran into issues. Have you heard about anything like this before with .forwardRef?

One last piece of info is that I kept my backup branch of not using .forwardRef with React Context (as mentioned earlier in this conversation), and when I used that, it worked perfectly in both dev and prod. So I do think this is .forwardRef-related.

@gaearon
Copy link
Collaborator

gaearon commented Aug 30, 2018

Do you mean you used TestUtils in production?

We’ll need a reproducing case if something didn’t work.

@noahgrant
Copy link
Author

No, sorry, this has nothing to do with TestUtils, this has to do with the React.Context HOC that returns a forwardRef as written in the first comment. I will try to repro, but I'm worried that it will be difficult because it's flakey—I have a few instances of about 100 in my app where it didn't work, and each one that didn't work is very different. So I'll need to spend more time on it to figure out how to replicate. Just wanted to raise it in case you'd seen/heard anything about it before and had any insight. It only happens in the production build.

@gaearon
Copy link
Collaborator

gaearon commented Aug 30, 2018

Not aware of any issues in the latest version.

@noahgrant
Copy link
Author

Digging in a little more, I believe the issue is from uglify, maybe at the compression step (when I use minimize: false in my webpack setup, the prod build operates correctly). I have run into issues with that before. If I can pinpoint exactly what it is, I'll make one more comment here just as a reference, but then I'll file with them.

@gaearon
Copy link
Collaborator

gaearon commented Aug 30, 2018

Unfortunately Uglify is notably buggy with ES6 input. I'd suggest always transpiling to ES5 before Uglifying.

@Jessidhia
Copy link
Contributor

The ES6 branch is no longer maintained; either switch to terser (fork of the ES6 branch of uglify) or always use uglify in ecma: 5 mode.

@noahgrant
Copy link
Author

if it's no longer maintained, why is webpack still using it?

@Jessidhia
Copy link
Contributor

The ES5 branch of uglify is still maintained. Webpack still uses it by default because of semver concerns; only webpack@5 can change what minifier is used by default. I don't know if terser will be used, but there's at least a will to decouple uglifyjs-webpack-plugin as a direct dependency of webpack core.

There is also https://github.com/webpack-contrib/terser-webpack-plugin

@noahgrant
Copy link
Author

i don't actually think that's true: https://webpack.js.org/plugins/uglifyjs-webpack-plugin/, and i know that from our experience, we are shipping minified ES6 code and up until now we haven't changed the default minifier settings—i imagine that using the ES5 branch to minify ES6 code wouldn't work.

@Jessidhia
Copy link
Contributor

Yes, because it'll still work if you happen to not be hitting the bugs that the uglify ES6 branch had.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants