Fix NextJS 500s (github#20048)

mikesurowiec · web-flow · commit a24d01f78a96 · 2021-06-22T17:30:40.000Z
* feat: allow server to contextualize request when on a /_next/data path

* add client side routing test, run lint

* enable nextjs client side routing
diff --git a/components/Link.tsx b/components/Link.tsx
@@ -4,7 +4,7 @@ import { useMainContext } from 'components/context/MainContext'
 
 const { NODE_ENV } = process.env
 
-const enableNextLinks = false
+const enableNextLinks = true
 
 type Props = { locale?: string } & ComponentProps<'a'>
 export function Link(props: Props) {
diff --git a/components/Survey.tsx b/components/Survey.tsx
@@ -128,7 +128,9 @@ export const Survey = () => {
         </>
       )}
 
-      {state === ViewState.END && <p className="color-text-secondary f6" data-testid="survey-end">{t`feedback`}</p>}
+      {state === ViewState.END && (
+        <p className="color-text-secondary f6" data-testid="survey-end">{t`feedback`}</p>
+      )}
     </form>
   )
 }
diff --git a/components/context/ProductSubLandingContext.tsx b/components/context/ProductSubLandingContext.tsx
@@ -62,7 +62,7 @@ export const getProductSubLandingContextFromRequest = (req: any): ProductSubLand
     includeGuides: (page.includeGuides || []).map((guide: any) => {
       return {
         ...pick(guide, ['href', 'title', 'intro', 'topics']),
-        type: guide.type || ''
+        type: guide.type || '',
       }
     }),
   }
diff --git a/components/sublanding/ArticleCard.tsx b/components/sublanding/ArticleCard.tsx
@@ -10,7 +10,9 @@ export const ArticleCard = ({ card, typeLabel }: Props) => {
     <div data-testid="article-card" className="d-flex col-12 col-md-4 pr-0 pr-md-6 pr-lg-8">
       <a className="no-underline d-flex flex-column py-3 border-bottom" href={card.href}>
         <h4 className="h4 color-text-primary mb-1">{card.title}</h4>
-        <div className="h6 text-uppercase" data-testid="article-card-type">{typeLabel}</div>
+        <div className="h6 text-uppercase" data-testid="article-card-type">
+          {typeLabel}
+        </div>
         <p className="color-text-secondary my-3">{card.intro}</p>
         {card.topics.length > 0 && (
           <div>
diff --git a/components/sublanding/SubLandingHero.tsx b/components/sublanding/SubLandingHero.tsx
@@ -9,7 +9,7 @@ export const SubLandingHero = () => {
   const { t } = useTranslation('product_sublanding')
 
   const guideItems = featuredTrack?.guides?.map((guide) => (
-    <li className="px-2 d-flex flex-shrink-0">
+    <li className="px-2 d-flex flex-shrink-0" key={guide.href}>
       <Link
         href={`${guide.href}?learn=${featuredTrack.trackName}`}
         className="d-inline-block Box p-5 color-bg-primary color-border-primary no-underline"
diff --git a/javascripts/wrap-code-terms.ts b/javascripts/wrap-code-terms.ts
@@ -11,9 +11,9 @@ export default function () {
   const codeTerms = document.querySelectorAll('#article-contents table code')
   if (!codeTerms) return
 
-  codeTerms.forEach(node => {
+  codeTerms.forEach((node) => {
     // Return early if a child node is an anchor element
-    const hasChildAnchor = Array.from(node.childNodes).some(child => child.nodeName === 'A')
+    const hasChildAnchor = Array.from(node.childNodes).some((child) => child.nodeName === 'A')
     if (hasChildAnchor) return
 
     // Do the wrapping on the inner text only
diff --git a/middleware/context.js b/middleware/context.js
@@ -34,14 +34,14 @@ module.exports = async function contextualize (req, res, next) {
   // e.g. searches for "req.context.page" will include results from this file
   req.context.currentLanguage = req.language
   req.context.userLanguage = req.userLanguage
-  req.context.currentVersion = getVersionStringFromPath(req.path)
-  req.context.currentProduct = getProductStringFromPath(req.path)
-  req.context.currentCategory = getCategoryStringFromPath(req.path)
+  req.context.currentVersion = getVersionStringFromPath(req.pagePath)
+  req.context.currentProduct = getProductStringFromPath(req.pagePath)
+  req.context.currentCategory = getCategoryStringFromPath(req.pagePath)
   req.context.productMap = productMap
   req.context.activeProducts = activeProducts
   req.context.allVersions = allVersions
-  req.context.currentPathWithoutLanguage = getPathWithoutLanguage(req.path)
-  req.context.currentPath = req.path
+  req.context.currentPathWithoutLanguage = getPathWithoutLanguage(req.pagePath)
+  req.context.currentPath = req.pagePath
   req.context.query = req.query
   req.context.languages = languages
   req.context.productNames = productNames
diff --git a/middleware/contextualizers/generic-toc.js b/middleware/contextualizers/generic-toc.js
@@ -23,7 +23,7 @@ module.exports = async function genericToc (req, res, next) {
   const currentTocType = tocTypes[req.context.page.documentType]
 
   // Find the part of the site tree that corresponds to the current path.
-  const treePage = findPageInSiteTree(req.context.currentProductTree, req.context.currentEnglishTree, req.path)
+  const treePage = findPageInSiteTree(req.context.currentProductTree, req.context.currentEnglishTree, req.pagePath)
 
   // Conditionally run getTocItems() recursively.
   let isRecursive
diff --git a/middleware/contextualizers/graphql.js b/middleware/contextualizers/graphql.js
@@ -13,7 +13,7 @@ module.exports = function graphqlContext (req, res, next) {
   const currentVersionObj = allVersions[req.context.currentVersion]
   // ignore requests to non-GraphQL reference paths
   // and to versions that don't exist
-  if (!req.path.includes('/graphql/') || !currentVersionObj) {
+  if (!req.pagePath.includes('/graphql/') || !currentVersionObj) {
     return next()
   }
   // Get the relevant name of the GraphQL schema files for the current version
diff --git a/middleware/contextualizers/release-notes.js b/middleware/contextualizers/release-notes.js
@@ -12,7 +12,7 @@ const supported = all.filter(release => {
 
 module.exports = async function releaseNotesContext (req, res, next) {
   // The `/release-notes` sub-path
-  if (!(req.path.endsWith('/release-notes') || req.path.endsWith('/admin'))) return next()
+  if (!(req.pagePath.endsWith('/release-notes') || req.pagePath.endsWith('/admin'))) return next()
 
   const [requestedPlan, requestedRelease] = req.context.currentVersion.split('@')
   const releaseNotesPerPlan = req.context.site.data['release-notes'][requestedPlan]
@@ -28,7 +28,7 @@ module.exports = async function releaseNotesContext (req, res, next) {
   if (hasNumberedReleases) {
     const currentReleaseNotes = releaseNotesPerPlan[`${requestedRelease.replace(/\./g, '-')}`]
 
-    if (!currentReleaseNotes && req.path.endsWith('/release-notes')) {
+    if (!currentReleaseNotes && req.pagePath.endsWith('/release-notes')) {
       // If the GHES version doesn't have any release notes, let's be helpful and redirect to `enterprise.github.com`
       return requestedPlan === 'enterprise-server'
         ? res.redirect(`https://enterprise.github.com/releases/${requestedRelease}.0/notes`)
diff --git a/middleware/contextualizers/rest.js b/middleware/contextualizers/rest.js
@@ -15,10 +15,10 @@ module.exports = function restContext (req, res, next) {
   ))
 
   // ignore requests to non-REST reference paths
-  if (!req.path.includes('rest/reference')) return next()
+  if (!req.pagePath.includes('rest/reference')) return next()
 
   // e.g. the `activity` from `/en/rest/reference/activity#events`
-  const category = req.path
+  const category = req.pagePath
     .split('rest/reference')[1]
     .replace(/^\//, '') // remove leading slash
     .split('/')[0]
diff --git a/middleware/contextualizers/webhooks.js b/middleware/contextualizers/webhooks.js
@@ -8,7 +8,7 @@ module.exports = function webhooksContext (req, res, next) {
   const currentVersionObj = allVersions[req.context.currentVersion]
   // ignore requests to non-webhook reference paths
   // and to versions that don't exist
-  if (!req.path.includes('webhook') || !currentVersionObj) {
+  if (!req.pagePath.includes('webhook') || !currentVersionObj) {
     return next()
   }
 
diff --git a/middleware/find-page.js b/middleware/find-page.js
@@ -1,11 +1,11 @@
 // This middleware uses the request path to find a page in the preloaded context.pages object
 
 module.exports = async function findPage (req, res, next) {
-  let page = req.context.pages[req.path]
+  let page = req.context.pages[req.pagePath]
 
   // if this is a localized request that can't be found, try finding an English variant
   if (!page && req.language !== 'en') {
-    const englishPath = req.path.replace(new RegExp(`^/${req.language}`), '/en')
+    const englishPath = req.pagePath.replace(new RegExp(`^/${req.language}`), '/en')
     // NOTE the fallback page will have page.languageCode = 'en'
     page = req.context.pages[englishPath]
   }
diff --git a/middleware/handle-next-data-path.js b/middleware/handle-next-data-path.js
@@ -0,0 +1,19 @@
+module.exports = async function handleNextDataPath (req, res, next) {
+  if (req.path.startsWith('/_next/data') && req.path.endsWith('.json')) { 
+    // translate a nextjs data request to a page path that the server can use on context 
+    // this is triggered via client-side route tranistions
+    // example path:
+    // /_next/data/development/en/free-pro-team%40latest/github/setting-up-and-managing-your-github-user-account.json
+    const decodedPath = decodeURIComponent(req.path)
+    const parts = decodedPath.split('/').slice(4)
+    // free-pro-team@latest should not be included in the page path
+    if (parts[1] === 'free-pro-team@latest') {
+      parts.splice(1,1)
+    }
+    req.pagePath = '/' + parts.join('/').replace(/.json+$/, '')
+  } else {
+    req.pagePath = req.path
+  }
+
+  next()
+}
diff --git a/middleware/index.js b/middleware/index.js
@@ -33,6 +33,7 @@ module.exports = function (app) {
   app.set('trust proxy', 1)
   app.use(require('./rate-limit'))
   app.use(instrument('./handle-invalid-paths'))
+  app.use(instrument('./handle-next-data-path'))
 
   // *** Security ***
   app.use(require('./cors'))
diff --git a/middleware/learning-track.js b/middleware/learning-track.js
@@ -20,7 +20,7 @@ module.exports = async function learningTrack (req, res, next) {
 
   const currentLearningTrack = { trackName }
 
-  const guidePath = getPathWithoutLanguage(getPathWithoutVersion(req.path))
+  const guidePath = getPathWithoutLanguage(getPathWithoutVersion(req.pagePath))
   const guideIndex = track.guides.findIndex((path) => path === guidePath)
 
   if (guideIndex < 0) return noTrack()
diff --git a/middleware/next.js b/middleware/next.js
@@ -11,7 +11,7 @@ if (FEATURE_NEXTJS) {
 }
 
 module.exports = function renderPageWithNext (req, res, next) {
-  if (req.path.startsWith('/_next/')) {
+  if (req.path.startsWith('/_next') && !req.path.startsWith('/_next/data')) {
     return nextHandleRequest(req, res)
   }
 
diff --git a/middleware/render-page.js b/middleware/render-page.js
@@ -131,7 +131,7 @@ module.exports = async function renderPage (req, res, next) {
   const context = Object.assign({}, req.context, { page })
 
   // collect URLs for variants of this page in all languages
-  context.page.languageVariants = Page.getLanguageVariants(req.path)
+  context.page.languageVariants = Page.getLanguageVariants(req.pagePath)
   // Stop processing if the connection was already dropped
   if (isConnectionDropped(req, res)) return
 
@@ -147,14 +147,14 @@ module.exports = async function renderPage (req, res, next) {
   }
 
   // handle special-case prerendered GraphQL objects page
-  if (req.path.endsWith('graphql/reference/objects')) {
+  if (req.pagePath.endsWith('graphql/reference/objects')) {
     // concat the markdown source miniToc items and the prerendered miniToc items
     context.miniTocItems = context.miniTocItems.concat(req.context.graphql.prerenderedObjectsForCurrentVersion.miniToc)
     context.renderedPage = context.renderedPage + req.context.graphql.prerenderedObjectsForCurrentVersion.html
   }
 
   // handle special-case prerendered GraphQL input objects page
-  if (req.path.endsWith('graphql/reference/input-objects')) {
+  if (req.pagePath.endsWith('graphql/reference/input-objects')) {
     // concat the markdown source miniToc items and the prerendered miniToc items
     context.miniTocItems = context.miniTocItems.concat(req.context.graphql.prerenderedInputObjectsForCurrentVersion.miniToc)
     context.renderedPage = context.renderedPage + req.context.graphql.prerenderedInputObjectsForCurrentVersion.html
@@ -164,7 +164,7 @@ module.exports = async function renderPage (req, res, next) {
   context.page.fullTitle = context.page.titlePlainText
 
   // add localized ` - GitHub Docs` suffix to <title> tag (except for the homepage)
-  if (!patterns.homepagePath.test(req.path)) {
+  if (!patterns.homepagePath.test(req.pagePath)) {
     context.page.fullTitle = context.page.fullTitle + ' - ' + context.site.data.ui.header.github_docs
   }
 
diff --git a/nodemon.json b/nodemon.json
@@ -6,6 +6,7 @@
     "translations",
     "dist",
     "javascripts",
-    "stylesheets"
+    "stylesheets",
+    "tests"
   ]
 }
diff --git a/pages/_app.tsx b/pages/_app.tsx
@@ -11,10 +11,8 @@ import events from 'javascripts/events'
 import experiment from 'javascripts/experiment'
 import setNextEnv from 'javascripts/set-next-env'
 
-
 type MyAppProps = AppProps & { csrfToken: string; themeProps: typeof defaultThemeProps }
 const MyApp = ({ Component, pageProps, csrfToken, themeProps }: MyAppProps) => {
-
   useEffect(() => {
     events()
     experiment()
diff --git a/tests/browser/browser.js b/tests/browser/browser.js
@@ -337,3 +337,25 @@ describe('nextjs query param', () => {
     flagVal === true ? expect(IS_NEXTJS_PAGE).toBe(true) : expect(IS_NEXTJS_PAGE).toBe(false)
   })
 })
+
+describe('next/link client-side navigation', () => {
+  jest.setTimeout(60 * 1000)
+
+  it('should have 200 response to /_next/data when link is clicked', async () => {
+    const initialViewport = page.viewport()
+    await page.setViewport({ width: 1024, height: 768 })
+    await page.goto('http://localhost:4001/en/actions')
+
+    const [response] = await Promise.all([
+      page.waitForResponse(
+        (response) =>
+          response.url().startsWith('http://localhost:4001/_next/data') 
+      ),
+      page.waitForNavigation({ waitUntil: 'networkidle2' }),
+      page.click('.sidebar-categories .sidebar-category:nth-child(2) a'),
+    ])
+
+    expect(response.status()).toBe(200)
+    await page.setViewport(initialViewport)
+  })
+})

Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,9 @@ export const Survey = () => {`
`128`	`128`	`</>`
`129`	`129`	`)}`
`130`	`130`
`131`		- {state === ViewState.END && <p className="color-text-secondary f6" data-testid="survey-end">{t`feedback`}</p>}
	`131`	`+ {state === ViewState.END && (`
	`132`	+ <p className="color-text-secondary f6" data-testid="survey-end">{t`feedback`}</p>
	`133`	`+ )}`
`132`	`134`	`</form>`
`133`	`135`	`)`
`134`	`136`	`}`
Original file line number	Diff line number	Diff line change
`@@ -62,7 +62,7 @@ export const getProductSubLandingContextFromRequest = (req: any): ProductSubLand`
`62`	`62`	`includeGuides: (page.includeGuides \|\| []).map((guide: any) => {`
`63`	`63`	`return {`
`64`	`64`	`...pick(guide, ['href', 'title', 'intro', 'topics']),`
`65`		`- type: guide.type \|\| ''`
	`65`	`+ type: guide.type \|\| '',`
`66`	`66`	`}`
`67`	`67`	`}),`
`68`	`68`	`}`
Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,7 @@ module.exports = function graphqlContext (req, res, next) {`
`13`	`13`	`const currentVersionObj = allVersions[req.context.currentVersion]`
`14`	`14`	`// ignore requests to non-GraphQL reference paths`
`15`	`15`	`// and to versions that don't exist`
`16`		`- if (!req.path.includes('/graphql/') \|\| !currentVersionObj) {`
	`16`	`+ if (!req.pagePath.includes('/graphql/') \|\| !currentVersionObj) {`
`17`	`17`	`return next()`
`18`	`18`	`}`
`19`	`19`	`// Get the relevant name of the GraphQL schema files for the current version`