Skip to content

Commit

Permalink
simplify find-page middleware (github#30642)
Browse files Browse the repository at this point in the history
  • Loading branch information
peterbe authored Sep 12, 2022
1 parent 86974cc commit 5f46725
Show file tree
Hide file tree
Showing 7 changed files with 13 additions and 93 deletions.
4 changes: 3 additions & 1 deletion .prettierignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
translations/
/translations/
includes/
data/release-notes/
script/bookmarklets/
/.next/
/.coverage
2 changes: 1 addition & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"files.exclude": {
"**/translations": true
"translations/**": true
},
"workbench.editor.enablePreview": false,
"workbench.editor.enablePreviewFromQuickOpen": false
Expand Down
32 changes: 1 addition & 31 deletions middleware/find-page.js
Original file line number Diff line number Diff line change
@@ -1,35 +1,5 @@
import getRedirect from '../lib/get-redirect.js'
// This middleware uses the request path to find a page in the preloaded context.pages object

export default function findPage(req, res, next) {
let page = req.context.pages[req.pagePath]

// When a user navigates to a translated page that doesn't yet exists
// we want to first check if there is an English page with the same relative
// path.
// If an exact match in English doesn't exist, the requested page might have
// a redirect configured to a new page. This happens when an English page is
// renamed and Crowdin hasn't synced the new file.
// In both cases, redirect to the English page. If we don't redirect most
// components won't refresh and everything except the article will render
// in req.language.
if (!page && 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]
const redirectToPath = getRedirect(englishPath, req.context)

// If the requested translated page has a 1-1 mapping in English,
// redirect to that English page
if (page) {
return res.redirect(302, englishPath)
}
// If the English file was renamed and has a redirect that matches the
// requested page's href, redirect to the new English href
if (redirectToPath) {
return res.redirect(302, redirectToPath)
}
}
const page = req.context.pages[req.pagePath]

if (page) {
req.context.page = page
Expand Down
53 changes: 3 additions & 50 deletions tests/translations/files.js
Original file line number Diff line number Diff line change
@@ -1,32 +1,7 @@
import languages from '../../lib/languages.js'
import { omit } from 'lodash-es'
import walk from 'walk-sync'
import { execSync } from 'child_process'
import { get } from '../helpers/e2etest.js'
import { jest } from '@jest/globals'

function difference (A, B) {
const A2 = new Set(A)
B.forEach(b => A2.delete(b))
return A2
}

const englishPaths = new Set(walk('content', {
directories: false,
ignore: ['**/README.md', 'search', 'early-access'],
}))

const nonEnglish = Object.entries(omit(languages, 'en'))

const langWalksTable = nonEnglish.map(([code, lang]) => [
code,
lang,
new Set(walk(`${lang.dir}/content`, {
directories: false,
ignore: ['**/README.md', 'search'],
}))
])

describe('files', () => {
jest.setTimeout(60 * 1000)

Expand All @@ -42,32 +17,10 @@ describe('files', () => {
// crowdin upload sources command fails if there are empty source files
// please refer to crowdin-support #117 for more details
it('should not contain empty files', () => {
const command = "find content data -type f -empty"
const emptyFiles = execSync(command).toString().split("\n")
const disallowedEmptyFiles = emptyFiles.filter(file => file.match(/\.(yml|md)$/))
const command = 'find content data -type f -empty'
const emptyFiles = execSync(command).toString().split('\n')
const disallowedEmptyFiles = emptyFiles.filter((file) => file.match(/\.(yml|md)$/))

expect(disallowedEmptyFiles).toEqual([])
})

test.each(langWalksTable)("falls back to the English page if it can't find a %s page", async (code, lang, langPaths) => {
const paths = [...difference(englishPaths, langPaths)]
.map(path => path.replace('/index.md', ''))
.map(path => path.replace('.md', ''))
for (const path of paths) {
const res = await get(`/${code}/${path}`)
expect(res.statusCode, path).toBeGreaterThanOrEqual(200)
expect(res.statusCode, path).toBeLessThanOrEqual(399)
}
})

test.each(langWalksTable)("only loads a %s page if there's an English page", async (code, lang, langPaths) => {
const paths = [...difference(langPaths, englishPaths)]
.map(path => path.replace('/index.md', ''))
.map(path => path.replace('.md', ''))
for (const path of paths) {
const res = await get(`/${code}/${path}`)
expect(res.statusCode, path).toBeGreaterThanOrEqual(300)
expect(res.statusCode, path).toBeLessThanOrEqual(499)
}
})
})
11 changes: 3 additions & 8 deletions tests/translations/frame.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { languageKeys } from '../../lib/languages.js'
import { blockIndex } from '../../middleware/block-robots.js'
import { getDOM } from '../helpers/e2etest.js'

const langs = languageKeys.filter(lang => lang !== 'en')
const langs = languageKeys.filter((lang) => lang !== 'en')

describe('frame', () => {
test.each(langs)('allows crawling of %s pages', async (lang) => {
Expand Down Expand Up @@ -43,9 +43,7 @@ describe('frame', () => {
test.each(langs)('loads the side bar via site tree in %s', async (lang) => {
const $en = await getDOM(`/en/get-started`)
const $ = await getDOM(`/${lang}/get-started`)
expect(
$(`a[href="/${lang}/get-started"]`).text()
).not.toEqual(
expect($(`a[href="/${lang}/get-started"]`).text()).not.toEqual(
$en(`a[href="/${lang}/get-started"]`).text()
)
})
Expand All @@ -54,11 +52,8 @@ describe('frame', () => {
test.skip.each(langs)('loads the survey via site data in %s', async (lang) => {
const $en = await getDOM(`/en`)
const $ = await getDOM(`/${lang}`)
expect(
$('[data-testid="survey-form"] h2').text()
).not.toEqual(
expect($('[data-testid="survey-form"] h2').text()).not.toEqual(
$en('[data-testid="survey-form"] h2').text()
)
})
})

2 changes: 1 addition & 1 deletion tests/translations/redirects.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { languageKeys } from '../../lib/languages.js'
import { get } from '../helpers/e2etest.js'
import { PREFERRED_LOCALE_COOKIE_NAME } from '../../lib/constants.js'

const langs = languageKeys.filter(lang => lang !== 'en')
const langs = languageKeys.filter((lang) => lang !== 'en')

describe('redirects', () => {
test.each(langs)('redirects to %s if accept-language', async (lang) => {
Expand Down
2 changes: 1 addition & 1 deletion tests/translations/search.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { languageKeys } from '../../lib/languages'
import { get } from '../helpers/e2etest.js'

const langs = languageKeys.filter(lang => lang !== 'en')
const langs = languageKeys.filter((lang) => lang !== 'en')

// Skipping for now, as we would need to download the indexes with LFS
// in Actions to run these. Explore again after ES switch over.
Expand Down

0 comments on commit 5f46725

Please sign in to comment.