Update url prop handling for pages with new data methods (vercel#10653)

* Make sure to show error when url prop is returned for a page * Update test and handle undefined pageProps * Handle empty props * Apply suggestions from code review Co-Authored-By: Tim Neutkens <[email protected]> * Update tests * Update to not add url prop for SSG/SSP pages * Update errsh for reserved prop * Update errsh wording some more * Update tests and to warn instead of error * Update reserved prop warning * Include page in url prop warning Co-authored-by: Tim Neutkens <[email protected]> Co-authored-by: Joe Haddad <[email protected]>
MoOx · Feb 26, 2020 · 395714a · 395714a
1 parent 364b4fe
commit 395714a
Show file tree

Hide file tree

Showing 8 changed files with 152 additions and 13 deletions.
diff --git a/errors/reserved-page-prop.md b/errors/reserved-page-prop.md
@@ -0,0 +1,9 @@
+# Reserved Page Prop
+
+#### Why This Error Occurred
+
+In a page's `getInitialProps` a reserved prop was returned. Currently the only reserved page prop is `url` for legacy reasons.
+
+#### Possible Ways to Fix It
+
+Change the name of the prop returned from `getInitialProps` to any other name.
diff --git a/packages/next/build/babel/plugins/next-ssg-transform.ts b/packages/next/build/babel/plugins/next-ssg-transform.ts
@@ -1,10 +1,12 @@
 import { NodePath, PluginObj } from '@babel/core'
 import * as BabelTypes from '@babel/types'
 import { SERVER_PROPS_SSG_CONFLICT } from '../../../lib/constants'
+import {
+  STATIC_PROPS_ID,
+  SERVER_PROPS_ID,
+} from '../../../next-server/lib/constants'
 
 const pageComponentVar = '__NEXT_COMP'
-const prerenderId = '__N_SSG'
-const serverPropsId = '__N_SSP'
 
 export const EXPORT_NAME_GET_STATIC_PROPS = 'unstable_getStaticProps'
 export const EXPORT_NAME_GET_STATIC_PATHS = 'unstable_getStaticPaths'
@@ -53,7 +55,7 @@ function decorateSsgExport(
           '=',
           t.memberExpression(
             t.identifier(pageComponentVar),
-            t.identifier(state.isPrerender ? prerenderId : serverPropsId)
+            t.identifier(state.isPrerender ? STATIC_PROPS_ID : SERVER_PROPS_ID)
           ),
           t.booleanLiteral(true)
         ),
@@ -80,7 +82,7 @@ function decorateSsgExport(
           '=',
           t.memberExpression(
             t.identifier((defaultSpecifier as any).local.name),
-            t.identifier(state.isPrerender ? prerenderId : serverPropsId)
+            t.identifier(state.isPrerender ? STATIC_PROPS_ID : SERVER_PROPS_ID)
           ),
           t.booleanLiteral(true)
         ),

diff --git a/packages/next/next-server/lib/constants.ts b/packages/next/next-server/lib/constants.ts
@@ -34,3 +34,5 @@ export const ROUTE_NAME_REGEX = /^static[/\\][^/\\]+[/\\]pages[/\\](.*)\.js$/
 export const SERVERLESS_ROUTE_NAME_REGEX = /^pages[/\\](.*)\.js$/
 export const TEMPORARY_REDIRECT_STATUS = 307
 export const PERMANENT_REDIRECT_STATUS = 308
+export const STATIC_PROPS_ID = '__N_SSG'
+export const SERVER_PROPS_ID = '__N_SSP'
diff --git a/packages/next/next-server/server/load-components.ts b/packages/next/next-server/server/load-components.ts
@@ -5,6 +5,8 @@ import {
   CLIENT_STATIC_FILES_PATH,
   REACT_LOADABLE_MANIFEST,
   SERVER_DIRECTORY,
+  STATIC_PROPS_ID,
+  SERVER_PROPS_ID,
 } from '../lib/constants'
 import { join } from 'path'
 import { requirePage } from './require'
@@ -16,6 +18,20 @@ export function interopDefault(mod: any) {
   return mod.default || mod
 }
 
+function addComponentPropsId(
+  Component: any,
+  getStaticProps: any,
+  getServerProps: any
+) {
+  // Mark the component with the SSG or SSP id here since we don't run
+  // the SSG babel transform for server mode
+  if (getStaticProps) {
+    Component[STATIC_PROPS_ID] = true
+  } else if (getServerProps) {
+    Component[SERVER_PROPS_ID] = true
+  }
+}
+
 export type ManifestItem = {
   id: number | string
   name: string
@@ -66,11 +82,24 @@ export async function loadComponents(
 ): Promise<LoadComponentsReturnType> {
   if (serverless) {
     const Component = await requirePage(pathname, distDir, serverless)
+    const {
+      unstable_getStaticProps,
+      unstable_getStaticPaths,
+      unstable_getServerProps,
+    } = Component
+
+    addComponentPropsId(
+      Component,
+      unstable_getStaticProps,
+      unstable_getServerProps
+    )
+
     return {
       Component,
       pageConfig: Component.config || {},
-      unstable_getStaticProps: Component.unstable_getStaticProps,
-      unstable_getStaticPaths: Component.unstable_getStaticPaths,
+      unstable_getStaticProps,
+      unstable_getStaticPaths,
+      unstable_getServerProps,
     } as LoadComponentsReturnType
   }
   const documentPath = join(
@@ -111,6 +140,18 @@ export async function loadComponents(
     interopDefault(AppMod),
   ])
 
+  const {
+    unstable_getServerProps,
+    unstable_getStaticProps,
+    unstable_getStaticPaths,
+  } = ComponentMod
+
+  addComponentPropsId(
+    Component,
+    unstable_getStaticProps,
+    unstable_getServerProps
+  )
+
   return {
     App,
     Document,
@@ -119,8 +160,8 @@ export async function loadComponents(
     DocumentMiddleware,
     reactLoadableManifest,
     pageConfig: ComponentMod.config || {},
-    unstable_getServerProps: ComponentMod.unstable_getServerProps,
-    unstable_getStaticProps: ComponentMod.unstable_getStaticProps,
-    unstable_getStaticPaths: ComponentMod.unstable_getStaticPaths,
+    unstable_getServerProps,
+    unstable_getStaticProps,
+    unstable_getStaticPaths,
   }
 }
diff --git a/packages/next/next-server/server/render.tsx b/packages/next/next-server/server/render.tsx
@@ -546,6 +546,19 @@ export async function renderToHTML(
     props.pageProps = data.props
     ;(renderOpts as any).pageData = props
   }
+
+  if (
+    !isSpr && // we only show this warning for legacy pages
+    !unstable_getServerProps &&
+    process.env.NODE_ENV !== 'production' &&
+    Object.keys(props?.pageProps || {}).includes('url')
+  ) {
+    console.warn(
+      `The prop \`url\` is a reserved prop in Next.js for legacy reasons and will be overridden on page ${pathname}\n` +
+        `See more info here: https://err.sh/zeit/next.js/reserved-page-prop`
+    )
+  }
+
   // We only need to do this if we want to support calling
   // _app's getInitialProps for getServerProps if not this can be removed
   if (isDataReq) return props

diff --git a/packages/next/pages/_app.tsx b/packages/next/pages/_app.tsx
@@ -42,8 +42,19 @@ export default class App<P = {}, CP = {}, S = {}> extends React.Component<
 
   render() {
     const { router, Component, pageProps } = this.props as AppProps<CP>
-    const url = createUrl(router)
-    return <Component {...pageProps} url={url} />
+
+    return (
+      <Component
+        {...pageProps}
+        {
+          // we don't add the legacy URL prop if it's using non-legacy
+          // methods like getStaticProps and getServerProps
+          ...(!((Component as any).__N_SSG || (Component as any).__N_SSP)
+            ? { url: createUrl(router) }
+            : {})
+        }
+      />
+    )
   }
 }
 

diff --git a/test/integration/getserverprops/test/index.test.js b/test/integration/getserverprops/test/index.test.js
@@ -23,6 +23,7 @@ const nextConfig = join(appDir, 'next.config.js')
 let app
 let appPort
 let buildId
+let stderr
 
 const expectedManifestRoutes = () => ({
   '/something': {
@@ -322,6 +323,31 @@ const runTests = (dev = false) => {
   })
 
   if (dev) {
+    it('should not show warning from url prop being returned', async () => {
+      const urlPropPage = join(appDir, 'pages/url-prop.js')
+      await fs.writeFile(
+        urlPropPage,
+        `
+        export async function unstable_getServerProps() {
+          return {
+            props: {
+              url: 'something'
+            }
+          }
+        }
+
+        export default ({ url }) => <p>url: {url}</p>
+      `
+      )
+
+      const html = await renderViaHTTP(appPort, '/url-prop')
+      await fs.remove(urlPropPage)
+      expect(stderr).not.toMatch(
+        /The prop `url` is a reserved prop in Next.js for legacy reasons and will be overridden on page \/url-prop/
+      )
+      expect(html).toMatch(/url:.*?something/)
+    })
+
     it('should show error for extra keys returned from getServerProps', async () => {
       const html = await renderViaHTTP(appPort, '/invalid-keys')
       expect(html).toContain(
@@ -365,8 +391,13 @@ const runTests = (dev = false) => {
 describe('unstable_getServerProps', () => {
   describe('dev mode', () => {
     beforeAll(async () => {
+      stderr = ''
       appPort = await findPort()
-      app = await launchApp(appDir, appPort)
+      app = await launchApp(appDir, appPort, {
+        onStderr(msg) {
+          stderr += msg
+        },
+      })
       buildId = 'development'
     })
     afterAll(() => killApp(app))
@@ -382,8 +413,13 @@ describe('unstable_getServerProps', () => {
         'utf8'
       )
       await nextBuild(appDir)
+      stderr = ''
       appPort = await findPort()
-      app = await nextStart(appDir, appPort)
+      app = await nextStart(appDir, appPort, {
+        onStderr(msg) {
+          stderr += msg
+        },
+      })
       buildId = await fs.readFile(join(appDir, '.next/BUILD_ID'), 'utf8')
     })
     afterAll(() => killApp(app))

diff --git a/test/integration/prerender/test/index.test.js b/test/integration/prerender/test/index.test.js
@@ -428,6 +428,31 @@ const runTests = (dev = false, looseMode = false) => {
     //   )
     // })
 
+    it('should not show warning from url prop being returned', async () => {
+      const urlPropPage = join(appDir, 'pages/url-prop.js')
+      await fs.writeFile(
+        urlPropPage,
+        `
+        export async function unstable_getStaticProps() {
+          return {
+            props: {
+              url: 'something'
+            }
+          }
+        }
+
+        export default ({ url }) => <p>url: {url}</p>
+      `
+      )
+
+      const html = await renderViaHTTP(appPort, '/url-prop')
+      await fs.remove(urlPropPage)
+      expect(stderr).not.toMatch(
+        /The prop `url` is a reserved prop in Next.js for legacy reasons and will be overridden on page \/url-prop/
+      )
+      expect(html).toMatch(/url:.*?something/)
+    })
+
     it('should always show fallback for page not in getStaticPaths', async () => {
       const html = await renderViaHTTP(appPort, '/blog/post-321')
       const $ = cheerio.load(html)