Revert "redirect to your preferred language (#25664)" (#25869)

This reverts commit a9947c0.

Author: Peter Bengtsson

components/page-header/LanguagePicker.tsx

@@ -1,14 +1,9 @@
 import { useRouter } from 'next/router'
-import Cookies from 'js-cookie'
-
 import { Link } from 'components/Link'
 import { useLanguages } from 'components/context/LanguagesContext'
 import { Picker } from 'components/ui/Picker'
 import { useTranslation } from 'components/hooks/useTranslation'
 
-// This value is replicated in two places! See middleware/detect-language.js
-const PREFERRED_LOCALE_COOKIE_NAME = 'preferredlang'
-
 type Props = {
   variant?: 'inline'
 }
@@ -27,22 +22,6 @@ export const LanguagePicker = ({ variant }: Props) => {
   // in a "denormalized" way.
   const routerPath = router.asPath.split('#')[0]
 
-  function rememberPreferredLanguage(code: string) {
-    try {
-      Cookies.set(PREFERRED_LOCALE_COOKIE_NAME, code, {
-        expires: 365,
-        secure: document.location.protocol !== 'http:',
-      })
-    } catch (err) {
-      // You can never be too careful because setting a cookie
-      // can fail. For example, some browser
-      // extensions disallow all setting of cookies and attempts
-      // at the `document.cookie` setter could throw. Just swallow
-      // and move on.
-      console.warn('Unable to set preferred language cookie', err)
-    }
-  }
-
   return (
     <Picker
       variant={variant}
@@ -54,13 +33,7 @@ export const LanguagePicker = ({ variant }: Props) => {
           text: lang.nativeName || lang.name,
           selected: lang === selectedLang,
           item: (
-            <Link
-              href={routerPath}
-              locale={lang.code}
-              onClick={() => {
-                rememberPreferredLanguage(lang.code)
-              }}
-            >
+            <Link href={routerPath} locale={lang.code}>
               {lang.nativeName ? (
                 <>
                   <span lang={lang.code}>{lang.nativeName}</span> (

lib/get-redirect.js

@@ -9,9 +9,9 @@ const nonEnterpriseDefaultVersionPrefix = `/${nonEnterpriseDefaultVersion}`
 
 // Return the new URI if there is one, otherwise return undefined.
 export default function getRedirect(uri, context) {
-  const { redirects, userLanguage } = context
+  const { redirects, pages } = context
 
-  let language = userLanguage || 'en'
+  let language = 'en'
   let withoutLanguage = uri
   if (languagePrefixRegex.test(uri)) {
     language = uri.match(languagePrefixRegex)[1]
@@ -109,7 +109,12 @@ export default function getRedirect(uri, context) {
   }
 
   if (basicCorrection) {
-    return getRedirect(basicCorrection, context) || basicCorrection
+    return (
+      getRedirect(basicCorrection, {
+        redirects,
+        pages,
+      }) || basicCorrection
+    )
   }
 
   if (withoutLanguage.startsWith('/admin/')) {

middleware/detect-language.js

@@ -1,17 +1,14 @@
-import languages, { languageKeys } from '../lib/languages.js'
+import libLanguages from '../lib/languages.js'
 import parser from 'accept-language-parser'
+const languageCodes = Object.keys(libLanguages)
 
 const chineseRegions = ['CN', 'HK']
 
-// This value is replicated in two places! See <LanguagePicker/> component.
-// Note, the only reason this is exported is to benefit the tests.
-export const PREFERRED_LOCALE_COOKIE_NAME = 'preferredlang'
-
 function translationExists(language) {
   if (language.code === 'zh') {
     return chineseRegions.includes(language.region)
   }
-  return languageKeys.includes(language.code)
+  return languageCodes.includes(language.code)
 }
 
 function getLanguageCode(language) {
@@ -20,41 +17,33 @@ function getLanguageCode(language) {
 
 function getUserLanguage(browserLanguages) {
   try {
+    let userLanguage = getLanguageCode(browserLanguages[0])
     let numTopPreferences = 1
     for (let lang = 0; lang < browserLanguages.length; lang++) {
       // If language has multiple regions, Chrome adds the non-region language to list
       if (lang > 0 && browserLanguages[lang].code !== browserLanguages[lang - 1].code)
         numTopPreferences++
       if (translationExists(browserLanguages[lang]) && numTopPreferences < 3) {
-        return getLanguageCode(browserLanguages[lang])
+        userLanguage = getLanguageCode(browserLanguages[lang])
+        break
       }
     }
+    return userLanguage
   } catch {
     return undefined
   }
 }
 
-function getUserLanguageFromCookie(req) {
-  const value = req.cookies[PREFERRED_LOCALE_COOKIE_NAME]
-  // But if it's a WIP language, reject it.
-  if (value && languages[value] && !languages[value].wip) {
-    return value
-  }
-}
-
 // determine language code from a path. Default to en if no valid match
 export function getLanguageCodeFromPath(path) {
   const maybeLanguage = (path.split('/')[path.startsWith('/_next/data/') ? 4 : 1] || '').slice(0, 2)
-  return languageKeys.includes(maybeLanguage) ? maybeLanguage : 'en'
+  return languageCodes.includes(maybeLanguage) ? maybeLanguage : 'en'
 }
 
 export default function detectLanguage(req, res, next) {
   req.language = getLanguageCodeFromPath(req.path)
   // Detecting browser language by user preference
-  req.userLanguage = getUserLanguageFromCookie(req)
-  if (!req.userLanguage) {
-    const browserLanguages = parser.parse(req.headers['accept-language'])
-    req.userLanguage = getUserLanguage(browserLanguages)
-  }
+  const browserLanguages = parser.parse(req.headers['accept-language'])
+  req.userLanguage = getUserLanguage(browserLanguages)
   return next()
 }

middleware/redirects/handle-redirects.js

@@ -1,6 +1,6 @@
 import patterns from '../../lib/patterns.js'
 import { URL } from 'url'
-import { pathLanguagePrefixed } from '../../lib/languages.js'
+import languages, { pathLanguagePrefixed } from '../../lib/languages.js'
 import getRedirect from '../../lib/get-redirect.js'
 import { cacheControlFactory } from '../cache-control.js'
 
@@ -13,7 +13,16 @@ export default function handleRedirects(req, res, next) {
 
   // blanket redirects for languageless homepage
   if (req.path === '/') {
-    const language = getLanguage(req)
+    let language = 'en'
+
+    // if set, redirect to user's preferred language translation or else English
+    if (
+      req.context.userLanguage &&
+      languages[req.context.userLanguage] &&
+      !languages[req.context.userLanguage].wip
+    ) {
+      language = req.context.userLanguage
+    }
 
     // Undo the cookie setting that CSRF sets.
     res.removeHeader('set-cookie')
@@ -61,12 +70,17 @@ export default function handleRedirects(req, res, next) {
     // needs to become `/en/authentication/connecting-to-github-with-ssh`
     const possibleRedirectTo = `/en${req.path}`
     if (possibleRedirectTo in req.context.pages) {
-      const language = getLanguage(req)
-
+      // As of Jan 2022 we always redirect to `/en` if the URL doesn't
+      // specify a language.  ...except for the root home page (`/`).
+      // It's unfortunate but that's how it currently works.
+      // It's tracked in #1145
+      // Perhaps a more ideal solution would be to do something similar to
+      // the code above for `req.path === '/'` where we look at the user
+      // agent for a header and/or cookie.
       // Note, it's important to use `req.url` here and not `req.path`
       // because the full URL can contain query strings.
       // E.g. `/foo?json=breadcrumbs`
-      redirect = `/${language}${req.url}`
+      redirect = `/en${req.url}`
     }
   }
 
@@ -98,13 +112,6 @@ export default function handleRedirects(req, res, next) {
   return res.redirect(permanent ? 301 : 302, redirect)
 }
 
-function getLanguage(req, default_ = 'en') {
-  // req.context.userLanguage, if it truthy, is always a valid supported
-  // language. It's whatever was in the user's request but filtered
-  // based on non-WIP languages in lib/languages.js
-  return req.context.userLanguage || default_
-}
-
 function usePermanentRedirect(req) {
   // If the redirect was to essentially swap `enterprise-server@latest`
   // for `enterprise-server@3.x` then, we definitely don't want to

tests/routing/redirects.js

@@ -2,14 +2,12 @@ import { fileURLToPath } from 'url'
 import path from 'path'
 import { isPlainObject } from 'lodash-es'
 import supertest from 'supertest'
-import { jest } from '@jest/globals'
-
 import createApp from '../../lib/app.js'
 import enterpriseServerReleases from '../../lib/enterprise-server-releases.js'
 import Page from '../../lib/page.js'
 import { get } from '../helpers/supertest.js'
 import versionSatisfiesRange from '../../lib/version-satisfies-range.js'
-import { PREFERRED_LOCALE_COOKIE_NAME } from '../../middleware/detect-language.js'
+import { jest } from '@jest/globals'
 
 const __dirname = path.dirname(fileURLToPath(import.meta.url))
 
@@ -134,28 +132,6 @@ describe('redirects', () => {
       expect(res.headers.location).toBe('/ja')
       expect(res.headers['cache-control']).toBe('private, no-store')
     })
-    test('homepage redirects to preferred language by cookie', async () => {
-      const res = await get('/', {
-        headers: {
-          Cookie: `${PREFERRED_LOCALE_COOKIE_NAME}=ja`,
-          'Accept-Language': 'es', // note how this is going to be ignored
-        },
-      })
-      expect(res.statusCode).toBe(302)
-      expect(res.headers.location).toBe('/ja')
-      expect(res.headers['cache-control']).toBe('private, no-store')
-    })
-    test('homepage redirects to preferred language by cookie if valid', async () => {
-      const res = await get('/', {
-        headers: {
-          Cookie: `${PREFERRED_LOCALE_COOKIE_NAME}=xy`,
-          'Accept-Language': 'ja', // note how this is going to be ignored
-        },
-      })
-      expect(res.statusCode).toBe(302)
-      expect(res.headers.location).toBe('/ja')
-      expect(res.headers['cache-control']).toBe('private, no-store')
-    })
   })
 
   describe('external redirects', () => {
@@ -173,63 +149,13 @@ describe('redirects', () => {
   })
 
   describe('localized redirects', () => {
-    const redirectFrom =
-      '/desktop/contributing-to-projects/changing-a-remote-s-url-from-github-desktop'
-    const redirectTo =
-      '/desktop/contributing-and-collaborating-using-github-desktop/working-with-your-remote-repository-on-github-or-github-enterprise/changing-a-remotes-url-from-github-desktop'
-
     test('redirect_from for renamed pages', async () => {
-      const { res } = await get(`/ja${redirectFrom}`)
+      const { res } = await get(
+        '/ja/desktop/contributing-to-projects/changing-a-remote-s-url-from-github-desktop'
+      )
       expect(res.statusCode).toBe(301)
-      const expected = `/ja${redirectTo}`
-      expect(res.headers.location).toBe(expected)
-    })
-
-    test('redirect_from for renamed pages by Accept-Language header', async () => {
-      const { res } = await get(redirectFrom, {
-        headers: {
-          'Accept-Language': 'ja',
-        },
-      })
-      expect(res.statusCode).toBe(302)
-      const expected = `/ja${redirectTo}`
-      expect(res.headers.location).toBe(expected)
-    })
-
-    test('redirect_from for renamed pages but ignore Accept-Language header if not recognized', async () => {
-      const { res } = await get(redirectFrom, {
-        headers: {
-          // None of these are recognized
-          'Accept-Language': 'sv,fr,gr',
-        },
-      })
-      expect(res.statusCode).toBe(302)
-      const expected = `/en${redirectTo}`
-      expect(res.headers.location).toBe(expected)
-    })
-
-    test('redirect_from for renamed pages but ignore unrecognized Accept-Language header values', async () => {
-      const { res } = await get(redirectFrom, {
-        headers: {
-          // Only the last one is recognized
-          'Accept-Language': 'sv,ja',
-        },
-      })
-      expect(res.statusCode).toBe(302)
-      const expected = `/ja${redirectTo}`
-      expect(res.headers.location).toBe(expected)
-    })
-
-    test('will inject the preferred language from cookie', async () => {
-      const { res } = await get(redirectFrom, {
-        headers: {
-          Cookie: `${PREFERRED_LOCALE_COOKIE_NAME}=ja`,
-          'Accept-Language': 'es', // note how this is going to be ignored
-        },
-      })
-      // 302 because the redirect depended on cookie
-      expect(res.statusCode).toBe(302)
-      const expected = `/ja${redirectTo}`
+      const expected =
+        '/ja/desktop/contributing-and-collaborating-using-github-desktop/working-with-your-remote-repository-on-github-or-github-enterprise/changing-a-remotes-url-from-github-desktop'
       expect(res.headers.location).toBe(expected)
     })
   })

tests/unit/get-redirect.js

@@ -148,18 +148,4 @@ describe('getRedirect basics', () => {
     // it already has the enterprise-server prefix.
     expect(getRedirect('/enterprise-server/foo', ctx)).toBe(`/en/enterprise-server@${latest}/bar`)
   })
-
-  it('should redirect according to the req.context.userLanguage', () => {
-    const ctx = {
-      pages: {},
-      redirects: {
-        '/foo': '/bar',
-      },
-      userLanguage: 'ja',
-    }
-    expect(getRedirect('/foo', ctx)).toBe(`/ja/bar`)
-    // falls back to 'en' if it's falsy
-    ctx.userLanguage = null
-    expect(getRedirect('/foo', ctx)).toBe(`/en/bar`)
-  })
 })