Upgrade to Next 9 #22

0xRAG · 2020-01-13T07:54:09Z

Edit: Fixed the type issue in setting up the test component in with-urql-client.spec.ts

Summary

Next 9 was recently released and has first-class TS types! None of the types we were previously using (NextContext, NextComponentClass, NextFC) exist in the same way this version, so I had to look at what's available and figure out a (hopefully) reasonable mapping. I tested this upgrade with the first example and it worked fine -- things started to fall apart when running the tests. Going to revisit this tomorrow with fresh eyes! For now, here's what I came up with:

`NextContext`

becomes NextPageContext, includes AppTree, and is no longer generalized
renamed NextContextWithAppTree to NextUrqlContext and extend NextPageContext, allowing us to remove AppTree

`NextComponentClass`, `NextFC`

consolidated into NextComponentType, a generalized intersection over React.ComponentType and getInitialProps, accepting type variables for context, initialProps, and props (in that order).
replaced NextFC in tests with NextComponentType
in function returned from withUrqlClient, replaced union over NextComponentClass and NextFC with NextPage; seemed to make sense given the parameter name is Page
updated withUrql to NextComponentType; had to switch order of type variables from <P, IP, C> to <C, IP, P>

Context

#20

Notes

The yarn.lock diff is HUGE. What's the process here? Add everything, or only the related package changes?

* Remove deprecated `@types/next` * Add `next@9.1.7` to devDependencies * Replace `NextComponentClass` | `NextFC` union describing `Page` with `NextPage` * Rename `NextContextWithAppTree` to `NextUrqlContext` * Make `NextUrqlContext` extend `NextPageContext` instead of `NextContext`

0xRAG

Added some comments to hopefully clarify my train of thought

0xRAG · 2020-01-13T07:56:22Z

__tests__/with-urql-client.spec.tsx

@@ -98,7 +96,7 @@ describe('withUrqlClient', () => {
      Component = withUrqlClient(ctx => ({
        url: 'http://localhost:3000',
        fetchOptions: {
-          headers: { Authorization: ctx.req.headers.cookie },
+          headers: { Authorization: ctx.req ? ctx.req.headers.cookie : null },


TS was complaining that req could be undefined here... which I thought was weird because that's not anything new. The previous context had an optional req property on it, but TS wasn't complaining then 🤔 Will take another look tomorrow

I wouldn't worry too much about it. This is the accurate type now, and actually should be what users use. For the first server-rendered result, ctx.req will be defined, but on subsequent client-side navigation you'll never actually get a req object, b/c no request was made to the server. This looks good to me ✅

0xRAG · 2020-01-13T07:56:49Z

index.d.ts

@@ -18,24 +18,21 @@ interface PageProps {
  pageProps?: WithUrqlClient;
 }

-export interface NextContextWithAppTree extends NextContext {
-  AppTree: React.ComponentType<any>;


Don't need AppTree anymore, it's included on NextPageContext now

0xRAG · 2020-01-13T07:57:31Z

index.d.ts


 declare const withUrqlClient: <T = any, IP = any>(
-  clientOptions: NextUrqlClientConfig,
+  clientConfig: NextUrqlClientConfig,


For consistency with parameter name in implementation

:chefs-kiss:

0xRAG · 2020-01-13T07:59:03Z

index.d.ts

-  Page:
-    | NextComponentClass<T & IP & WithUrqlClient, IP>
-    | NextFC<T & IP & WithUrqlClient, IP>,


Consolidate into NextPage

0xRAG · 2020-01-13T07:59:56Z

index.d.ts

-) => NextFC<
-  T & IP & WithUrqlClient & WithUrqlInitialProps & PageProps,
+  Page: NextPage<T & IP & WithUrqlClient, IP>,
+) => NextComponentType<


Update NextFC to NextComponentType, who's type variables are in the reverse order of NextFC

Love when generic order gets changed around like that :oof:

0xRAG · 2020-01-13T08:00:43Z

src/with-urql-client.tsx

@@ -29,14 +29,13 @@ interface PageProps {
  pageProps?: WithUrqlClient;
 }

-export interface NextContextWithAppTree extends NextContext {
-  AppTree: React.ComponentType<any>;


Don't need AppTree anymore, it's included on NextPageContext now

0xRAG · 2020-01-13T08:01:01Z

src/with-urql-client.tsx

-    Page:
-      | NextComponentClass<T & IP & WithUrqlClient, IP>
-      | NextFC<T & IP & WithUrqlClient, IP>,


Consolidate into NextPage

0xRAG · 2020-01-13T08:01:19Z

src/with-urql-client.tsx

-    const withUrql: NextFC<
-      T & IP & WithUrqlClient & WithUrqlInitialProps & PageProps,
+  return (Page: NextPage<T & IP & WithUrqlClient, IP>) => {
+    const withUrql: NextComponentType<


Update NextFC to NextComponentType, who's type variables are in the reverse order of NextFC

parkerziegler · 2020-01-15T17:40:14Z

__tests__/with-urql-client.spec.tsx

@@ -1,12 +1,10 @@
 import React from 'react';
 import { shallow, configure } from 'enzyme';
 import Adapter from 'enzyme-adapter-react-16';
-import { NextFC } from 'next';
+import { NextComponentType } from 'next/types';


I think we should be able to just import this directly from next. They should be moving their types to a top-level lib export:

Suggested change

import { NextComponentType } from 'next/types';

import { NextComponentType } from 'next';

parkerziegler · 2020-01-15T17:46:09Z

__tests__/with-urql-client.spec.tsx


    // Fake up a string to simulate, say, a token accessed via browser cookies or localStorage.
    const token = Math.random()
      .toString(36)
      .slice(-10);

-    const mockContext: NextContextWithAppTree = {
+    const mockContext: NextUrqlContext = {
      AppTree: MockAppTree,
      pathname: '/',
      query: {


FYI I'm also seeing a TS error on L87 (in the new diff):

Type '{ headers: { cookie: string; }; }' is missing the following properties from type 'IncomingMessage': httpVersion, httpVersionMajor, httpVersionMinor, complete, and 40 more.

We'll just have to coerce the req type like so:

req: { headers: { cookie: token, }, } as NextUrqlContext['req']

Oh cool, didn't know about accessing a type property like that!

parkerziegler · 2020-01-15T17:49:01Z

src/with-urql-client.tsx

@@ -1,5 +1,5 @@
 import React from 'react';
-import { NextComponentClass, NextFC, NextContext } from 'next';
+import { NextPage, NextPageContext, NextComponentType } from 'next/types';


+1 to just importing from next directly.

parkerziegler · 2020-01-15T17:56:19Z

src/with-urql-client.tsx

@@ -77,7 +72,7 @@ function withUrqlClient<T = any, IP = any>(
      );
    };

-    withUrql.getInitialProps = async (ctx: NextContextWithAppTree) => {
+    withUrql.getInitialProps = async (ctx: NextUrqlContext) => {


So I think this type should actually be just:

Suggested change

withUrql.getInitialProps = async (ctx: NextUrqlContext) => {

withUrql.getInitialProps = async (ctx: NextPageContext) => {

the reason being that, at this point in the function's execution, we can't safely access ctx.urqlClient b/c it doesn't exist yet (it gets added on L83).

Ofc, that'll result in a type error, so on L83 you'll need to do:

(ctx as NextUrqlContext).urqlClient = urqlClient;

It's a small thing, but I think it better signals to future readers what we're doing here, which is actually shoving a property onto the original ctx object so that components lower in the tree can access it.

Ohh I see, makes sense. Would it also be helpful to coerce ctx as NextUrqlContext when we pass it to Page.getInitialProps on L89?

Ah yep, good catch – that way if a user wants to access ctx.urqlClient in their own getInitialProps function they can. Let's do that, and then I'll merge this!

Yup, and I figure it's at least more accurate! Added that in 👍

parkerziegler

Looking real good to me @ryan-gilb! Left some small suggestions in there – once we get those in I'll hit the ✅

0xRAG · 2020-01-16T12:06:12Z

Thanks for the feedback @parkerziegler 🙌 I implemented your suggestions and added some more coercion to with-urql-client.spec.ts to get it to run 😬

Also left a question expanding on signaling intent to readers, let me know your thoughts!

❤️

parkerziegler · 2020-01-18T03:15:11Z

Woohoo! Merge ahoy!

0xRAG commented Jan 13, 2020

View reviewed changes

0xRAG requested a review from parkerziegler January 13, 2020 08:07

parkerziegler reviewed Jan 15, 2020

View reviewed changes

parkerziegler approved these changes Jan 15, 2020

View reviewed changes

Ryan Gilbert added 3 commits January 16, 2020 04:36

Import types directly from top-level next

73f92b6

Coerce mock request object

a4a65de

Fix withUrqlClient test config

2e8a3b0

0xRAG changed the title ~~Upgrade to Next 9 (test fixes WIP)~~ Upgrade to Next 9 Jan 16, 2020

Coerce ctx into more accurate representation

121015b

parkerziegler merged commit eb13ec3 into master Jan 18, 2020

parkerziegler deleted the upgrade-next-types branch January 18, 2020 03:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Next 9 #22

Upgrade to Next 9 #22

0xRAG commented Jan 13, 2020 •

edited

Loading

0xRAG left a comment

0xRAG Jan 13, 2020

parkerziegler Jan 15, 2020

0xRAG Jan 13, 2020

0xRAG Jan 13, 2020

parkerziegler Jan 15, 2020

0xRAG Jan 13, 2020

0xRAG Jan 13, 2020

parkerziegler Jan 15, 2020

0xRAG Jan 13, 2020

0xRAG Jan 13, 2020

0xRAG Jan 13, 2020

parkerziegler Jan 15, 2020

parkerziegler Jan 15, 2020

0xRAG Jan 16, 2020

parkerziegler Jan 15, 2020

parkerziegler Jan 15, 2020 •

edited

Loading

0xRAG Jan 16, 2020

parkerziegler Jan 17, 2020

0xRAG Jan 17, 2020

parkerziegler left a comment

0xRAG commented Jan 16, 2020

parkerziegler commented Jan 18, 2020

	import { NextComponentType } from 'next/types';
	import { NextComponentType } from 'next';

	withUrql.getInitialProps = async (ctx: NextUrqlContext) => {
	withUrql.getInitialProps = async (ctx: NextPageContext) => {

Upgrade to Next 9 #22

Upgrade to Next 9 #22

Conversation

0xRAG commented Jan 13, 2020 • edited Loading

Summary

NextContext

NextComponentClass, NextFC

Context

Notes

0xRAG left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkerziegler Jan 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkerziegler left a comment

Choose a reason for hiding this comment

0xRAG commented Jan 16, 2020

parkerziegler commented Jan 18, 2020

0xRAG commented Jan 13, 2020 •

edited

Loading

`NextContext`

`NextComponentClass`, `NextFC`

parkerziegler Jan 15, 2020 •

edited

Loading