Skip to content

Change page export validity check on client and server in development #5857

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

Merged
merged 17 commits into from
Dec 17, 2018
Merged

Conversation

kylemh
Copy link
Contributor

@kylemh kylemh commented Dec 11, 2018

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:

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

some test:

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/)
})

@timneutkens
Copy link
Member

Yes we should definitely add that test. Also a test for memo( would make sense

@timneutkens
Copy link
Member

So these two fail:

✕ should recover after exporting an invalid page (30575ms)
✕ should recover after a bad return from the render function (30526ms)

You can run them using:

yarn testonly --testPathPattern="basic" -t "should recover after exporting an invalid page"

@elliottsj
Copy link
Contributor

Thanks for this 👍

I think we also need to update lib/router/router.js: https://github.com/zeit/next.js/blob/82c5cc38d59e65d98c74f5012c3780e2832880c6/packages/next-server/lib/router/router.js#L263-L265

@kylemh
Copy link
Contributor Author

kylemh commented Dec 11, 2018

-_-

the tests are failing, but the error is firing correctly?

kylemh and others added 3 commits December 15, 2018 15:06
`undefined` is not handled correctly by HMR, so revert this change in favor of the original values
@timneutkens
Copy link
Member

I just investigated why the tests failed and realized that you had changed the value of export default "something" to export default undefined incidentally this exposed a bug in HMR where if undefined is the component being exported we don't apply the changes. It might be interesting to investigate this further, but for the current PR I've changed undefined to {}

@timneutkens timneutkens merged commit 72e7929 into vercel:canary Dec 17, 2018
@Enalmada
Copy link
Contributor

I am trying to upgrade my reason-react project from 7.0.2 to latest canary and getting The default export is not a React Component in page: "/". The error seems related to things talked about in this ticket...if that is true, could you give me your advice on this issue:
#5929

@lock lock bot locked as resolved and limited conversation to collaborators Dec 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Home-rolled "is Component?" test is throwing false negatives for React 16.3.0 features
4 participants