Migrate next/router to use React.createContext #6030

alexandernanberg · 2019-01-11T08:32:21Z

Fixes parts of #5716. I had some issues with the test suite but I'm fairly certain that I got it working correctly.

packages/next/package.json

alexandernanberg · 2019-01-11T12:53:36Z

Got any tips on how to troubleshoot if the tests are failing? The test suite keeps spawning Chrome instances 😅

Edit: Looks like RouterContext exported from next/router is undefined 🤔

timneutkens · 2019-01-11T13:19:36Z

TypeError: Cannot read property 'Consumer' of undefined
    at WithRouteWrapper (/home/circleci/repo/test/integration/basic/.next/server/static/development/pages/nav/with-hoc.js:551:129)
    at processChild (/home/circleci/repo/node_modules/react-dom/cjs/react-dom-server.node.development.js:2790:14)
    at resolve (/home/circleci/repo/node_modules/react-dom/cjs/react-dom-server.node.development.js:2714:5)
    at ReactDOMServerRenderer.render (/home/circleci/repo/node_modules/react-dom/cjs/react-dom-server.node.development.js:3098:22)
    at ReactDOMServerRenderer.read (/home/circleci/repo/node_modules/react-dom/cjs/react-dom-server.node.development.js:3057:29)
    at renderToString (/home/circleci/repo/node_modules/react-dom/cjs/react-dom-server.node.development.js:3524:27)
    at render (/home/circleci/repo/packages/next-server/dist/server/render.js:39:16)
    at renderPage (/home/circleci/repo/packages/next-server/dist/server/render.js:96:16)
    at Function.getInitialProps (/home/circleci/repo/test/integration/basic/.next/server/static/development/pages/_document.js:1262:25)
    at Object.loadGetInitialProps (/home/circleci/repo/packages/next-server/dist/lib/utils.js:44:35)
    at Object.renderToHTML (/home/circleci/repo/packages/next-server/dist/server/render.js:99:36)
    at <anonymous>

Looks like an issue with the Consumer

timneutkens · 2019-01-11T13:20:26Z

You can run it locally:

./node_modules/next/dist/bin/next ./test/integration/basic

alexandernanberg · 2019-01-11T13:46:43Z

Ah nice thanks! Okay so I think the problem is that the Provider with the context isn't rendered on the server. I guess the client/index isn't rendered during SSR? Where would be the best place for the provider to be located?

…rg/next.js into use-new-context-in-router

timneutkens · 2019-01-11T13:54:01Z

Oh yeah I see, you didn't add it to packages/next-server/server/render.tsx 🕵️

alexandernanberg · 2019-01-11T15:01:32Z

Looks like the Azure build error is unrelated to this PR, right? 🤔

timneutkens · 2019-01-11T15:03:08Z

@alexandernanberg yep it fails inconsistently sometimes.

timneutkens · 2019-01-11T15:05:08Z

Great PR @alexandernanberg 🙏

timneutkens · 2019-01-11T15:05:21Z

Could you send me a DM on https://spectrum.chat/users/timneutkens

alexandernanberg · 2019-01-11T16:49:21Z

Done ✔️

jaydenseric · 2019-01-12T01:56:16Z

I'm trying to use RouterContext with next@8.0.0-canary.6, but the router is always undefined:

import { RouterContext } from 'next/router'

const Foo = () => (
  <RouterContext.Consumer>
    {router => {
      console.log(router) // undefined
      return null
    }}
  </RouterContext.Consumer>
)

Can you think why?

jaydenseric · 2019-01-12T02:06:33Z

packages/next/client/index.js

@@ -180,7 +180,9 @@ async function doRender ({ App, Component, props, err, emitter: emitterProp = em
  // In development runtime errors are caught by react-error-overlay.
  if (process.env.NODE_ENV === 'development') {
    renderReactElement((
-      <App {...appProps} />
+      <RouterContext.Provider value={makePublicRouterInstance(router)}>


Ok, so I think this is a big breaking change for next-graphql-react, and probably the Apollo examples too:

https://github.com/jaydenseric/next-graphql-react/blob/v1.0.1/src/index.mjs#L92

Are we sure this new approach of <RouterContext.Provider> wrapping <App> (as opposed to doing it inside <App> via a router prop, like before) is here to stay? If so, I can work it in to next-graphql-react and cut a new release.

I'm not sure I fully understand, the router props is still passed to the <App /> if that's what you are referring to 🤔

The way @alexandernanberg is exactly how I would have implemented it, outside of _app.js.

alexandernanberg · 2019-01-12T09:19:30Z

@jaydenseric That's interesting, I can't seem to reproduce it myself. Can you provide reproducible sample repo?

jaydenseric · 2019-01-12T22:56:18Z

@alexandernanberg in the with-graphql-react example, replace the contents of pages/index.js with:

import { Query } from 'graphql-react'
import { RouterContext } from 'next/router'

export default () => (
  <RouterContext.Consumer>
    {router => {
      console.log(router)
      return null
    }}
  </RouterContext.Consumer>
)

With npm run dev, in terminal you will see first undefined (bad) for the GraphQL preload render and then a router instance when the final SSR happens:

I imagine this reproduction would work for the with-apollo examples too:

https://github.com/zeit/next.js/blob/v8.0.0-canary.6/examples/with-apollo/lib/with-apollo-client.js#L27

* Revert #6030 * Fix _app childContextTypes

Alexander Nanberg added 3 commits January 11, 2019 09:10

Migrate router to use new context API

34ffeb7

Add missing Provider in dev mode

ee10909

Bump react peerDepencency version to 16.6.0

39f6446

timneutkens reviewed Jan 11, 2019

View reviewed changes

packages/next/package.json Outdated Show resolved Hide resolved

Alexander Nanberg added 3 commits January 11, 2019 13:40

Merge branch 'canary' into use-new-context-in-router

ac721d7

Refactor WithRouteWrapper to a function component

927551b

Change react peerDependency version to 16.3.0

7ccccff

Alexander Nanberg and others added 2 commits January 11, 2019 13:55

Correctly pass props to ComposedComponent

da170c5

Merge branch 'canary' into use-new-context-in-router

6166dfc

Fix RouterContext import

fcede7e

Merge branch 'use-new-context-in-router' of github.com:alexandernanbe…

b1c8837

…rg/next.js into use-new-context-in-router

Add RouterContext in SSR

86d8c0e

timneutkens approved these changes Jan 11, 2019

View reviewed changes

timneutkens merged commit 25fb3f9 into vercel:canary Jan 11, 2019

jaydenseric reviewed Jan 12, 2019

View reviewed changes

alexandernanberg deleted the use-new-context-in-router branch January 12, 2019 11:53

jaydenseric mentioned this pull request Jan 12, 2019

Router context breaking change in v8.0.0-canary.6 #6042

Closed

alexandernanberg restored the use-new-context-in-router branch January 14, 2019 12:31

alexandernanberg pushed a commit to alexandernanberg/next.js that referenced this pull request Jan 14, 2019

Revert vercel#6030

029dba3

timneutkens pushed a commit that referenced this pull request Jan 14, 2019

Revert #6030 (#6052)

7e7be0e

* Revert #6030 * Fix _app childContextTypes

lock bot locked as resolved and limited conversation to collaborators Jan 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate next/router to use React.createContext #6030

Migrate next/router to use React.createContext #6030

alexandernanberg commented Jan 11, 2019

alexandernanberg commented Jan 11, 2019 •

edited

Loading

timneutkens commented Jan 11, 2019

timneutkens commented Jan 11, 2019

alexandernanberg commented Jan 11, 2019

timneutkens commented Jan 11, 2019

alexandernanberg commented Jan 11, 2019

timneutkens commented Jan 11, 2019

timneutkens commented Jan 11, 2019

timneutkens commented Jan 11, 2019

alexandernanberg commented Jan 11, 2019

jaydenseric commented Jan 12, 2019

jaydenseric Jan 12, 2019

jaydenseric Jan 12, 2019 •

edited

Loading

alexandernanberg Jan 12, 2019

timneutkens Jan 12, 2019

alexandernanberg commented Jan 12, 2019 •

edited

Loading

jaydenseric commented Jan 12, 2019 •

edited

Loading

Migrate next/router to use React.createContext #6030

Migrate next/router to use React.createContext #6030

Conversation

alexandernanberg commented Jan 11, 2019

alexandernanberg commented Jan 11, 2019 • edited Loading

timneutkens commented Jan 11, 2019

timneutkens commented Jan 11, 2019

alexandernanberg commented Jan 11, 2019

timneutkens commented Jan 11, 2019

alexandernanberg commented Jan 11, 2019

timneutkens commented Jan 11, 2019

timneutkens commented Jan 11, 2019

timneutkens commented Jan 11, 2019

alexandernanberg commented Jan 11, 2019

jaydenseric commented Jan 12, 2019

jaydenseric Jan 12, 2019

Choose a reason for hiding this comment

jaydenseric Jan 12, 2019 • edited Loading

Choose a reason for hiding this comment

alexandernanberg Jan 12, 2019

Choose a reason for hiding this comment

timneutkens Jan 12, 2019

Choose a reason for hiding this comment

alexandernanberg commented Jan 12, 2019 • edited Loading

jaydenseric commented Jan 12, 2019 • edited Loading

alexandernanberg commented Jan 11, 2019 •

edited

Loading

jaydenseric Jan 12, 2019 •

edited

Loading

alexandernanberg commented Jan 12, 2019 •

edited

Loading

jaydenseric commented Jan 12, 2019 •

edited

Loading