Skip to content

Home-rolled "is Component?" test is throwing false negatives for React 16.3.0 features #4055

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

Closed
1 task done
jamesreggio opened this issue Mar 26, 2018 · 3 comments · Fixed by #5857
Closed
1 task done
Labels
good first issue Easy to fix issues, good for newcomers

Comments

@jamesreggio
Copy link
Contributor

  • I have searched the issues of this repository and believe that this is not a duplicate.

Overview

React 16.3 will include a utility for ref forwarding called React.forwardRef. Its RFC has been ratified and the initial implementation is available in alpha builds of 16.3.

The typeof objects returned by forwardRef happens to be "object", not "function", which means that if you wrap your Page component in a HOC that uses forwardRef, it will fail the overly-aggressive test here.

Expected Behavior

If I wrap a top-level Page component with React.forwardRef, I expect Next.js to render it without issue.

Current Behavior

If I wrap a top-level Page component with React.forwardRef, Next.js throws the following error:

Error: The default export is not a React Component in page: ...

Steps to Reproduce (for bugs)

For any Page component in a Next.js app, add the following lines:

// ...
const MyPage = () => (...);
export default React.forwardRef((props, ref) => <MyPage {...props} forwardedRef={ref} />);

Context

As soon as the forwardRef API is released, library that expose HOCs are going to start using it to provide a standard way of accessing the ref of the wrapped component. (Prior to this, each HOC library would take a slightly different approach.)

As more libraries and apps are updated, the number of people impacted will grow. I expect this to happen fairly rapidly, at least for actively-maintained libraries.

Your Environment

Tech Version
next 5.0.1-canary.17
@elliottsj
Copy link
Contributor

I'm also experiencing this, using Next.js v6.1.1.

Based on facebook/react#12453, it sounds like the way forward is to replace the current check with ReactIs.isValidElementType(Component): https://github.com/facebook/react/tree/v16.4.2/packages/react-is#determining-if-a-component-is-valid

I'm trying this out now and it appears to work well. Pull request coming.

@elliottsj
Copy link
Contributor

Pull request is here: #5095

FWIW, I found this issue using react-cookie, using this workaround in the meantime:

// pages/index.js

const AppWithCookies = withCookies(App);
export default () => <AppWithCookies />;

// compared to this, which produces an error:
// export default withCookies(App);

elliottsj added a commit to elliottsj/next.js that referenced this issue Sep 5, 2018
elliottsj added a commit to elliottsj/next.js that referenced this issue Sep 5, 2018
elliottsj added a commit to elliottsj/next.js that referenced this issue Sep 6, 2018
@timneutkens
Copy link
Member

#5095 implement pretty much all that is needed for this to work, it had some merge conflicts however. So if someone wants to take on this issue feel free to 🙏

@timneutkens timneutkens added help wanted good first issue Easy to fix issues, good for newcomers labels Dec 10, 2018
elliottsj added a commit to elliottsj/next.js that referenced this issue Dec 11, 2018
elliottsj added a commit to elliottsj/next.js that referenced this issue Dec 11, 2018
Fixes vercel#4055

Check isValidElementType only in development

Ignore react-is in production
timneutkens pushed a commit that referenced this issue Dec 17, 2018
…#5857)

Resolves #4055 

Credit: #5095

I didn't use the ignore webpack plugin from the original PR and tested bundle size with #5339 - seems to be safe on that front.

Was able to get tests to pass locally, unsure of what goes wrong in CI 🤷‍♂️ 

**Questions**
1) The initial PR didn't include changes to `next-server/lib/router` in `getRouteInfo()`. Should the same changes be made within?

2) Should we add a test for rendering a component created via `forwardRef()`?

`component-with-forwardedRef`:
```javascript
export default React.forwardRef((props, ref) => <span {...props} forwardedRef={ref}>This is a component with a forwarded ref</span>);
```

some test:
```javascript
test('renders from forwardRef', async () => {
  const $ = await get$('/component-with-forwardedRef')
  const span = $('span')
  expect(span.text()).toMatch(/This is a component with a forwarded ref/)
})
```
@lock lock bot locked as resolved and limited conversation to collaborators Dec 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Easy to fix issues, good for newcomers
Projects
None yet
3 participants