From edc25dd421ead43f461ccfa5d444aa638afccac6 Mon Sep 17 00:00:00 2001 From: Peter Bengtsson Date: Tue, 13 Sep 2022 15:06:33 +0200 Subject: [PATCH] re-read content on every request (#30646) --- contributing/development.md | 2 +- middleware/find-page.js | 55 +++++++++++++++++- middleware/index.js | 2 +- nodemon.json | 3 +- tests/unit/find-page-middleware.js | 89 ++++++++++++++++++++++++++++++ 5 files changed, 146 insertions(+), 5 deletions(-) create mode 100644 tests/unit/find-page-middleware.js diff --git a/contributing/development.md b/contributing/development.md index d7ed1b1d253f..b95c9ed627e5 100644 --- a/contributing/development.md +++ b/contributing/development.md @@ -22,7 +22,7 @@ npm run build npm start ``` -You should now have a running server! Visit [localhost:4000](http://localhost:4000) in your browser. It will automatically restart as you make changes to site content. +You should now have a running server! Visit [localhost:4000](http://localhost:4000) in your browser. When you're ready to stop your local server, type Ctrl+C in your terminal window. diff --git a/middleware/find-page.js b/middleware/find-page.js index a5bd431af06d..fc2a609bebc5 100644 --- a/middleware/find-page.js +++ b/middleware/find-page.js @@ -1,5 +1,32 @@ -export default function findPage(req, res, next) { - const page = req.context.pages[req.pagePath] +import { fileURLToPath } from 'url' +import path from 'path' +import { existsSync } from 'fs' + +import Page from '../lib/page.js' +import { languageKeys } from '../lib/languages.js' + +const languagePrefixRegex = new RegExp(`^/(${languageKeys.join('|')})(/|$)`) +const englishPrefixRegex = /^\/en(\/|$)/ +const __dirname = path.dirname(fileURLToPath(import.meta.url)) +const CONTENT_ROOT = path.posix.join(__dirname, '..', 'content') + +export default async function findPage( + req, + res, + next, + // Express won't execute these but it makes it easier to unit test + // the middleware. + { isDev = process.env.NODE_ENV === 'development', contentRoot = CONTENT_ROOT } = {} +) { + // Filter out things like `/will/redirect` or `/_next/data/...` + if (!languagePrefixRegex.test(req.pagePath)) { + return next() + } + + const page = + isDev && englishPrefixRegex.test(req.pagePath) + ? await rereadByPath(req.pagePath, contentRoot, req.context.currentVersion) + : req.context.pages[req.pagePath] if (page) { req.context.page = page @@ -8,3 +35,27 @@ export default function findPage(req, res, next) { return next() } + +async function rereadByPath(uri, contentRoot, currentVersion) { + const languageCode = uri.match(languagePrefixRegex)[1] + const withoutLanguage = uri.replace(languagePrefixRegex, '/') + const withoutVersion = withoutLanguage.replace(`/${currentVersion}/`, '/') + // TODO: Support loading translations the same way. + // NOTE: No one is going to test translations like this in development + // but perhaps one day we can always and only do these kinds of lookups + // at runtime. + const possible = path.join(contentRoot, withoutVersion) + const filePath = existsSync(possible) ? path.join(possible, 'index.md') : possible + '.md' + const relativePath = path.relative(contentRoot, filePath) + const basePath = contentRoot + + // Remember, the Page.init() can return a Promise that resolves to falsy + // if it can't read the file in from disk. E.g. a request for /en/non/existent. + // In other words, it's fine if it can't be read from disk. It'll get + // handled and turned into a nice 404 message. + return await Page.init({ + basePath, + relativePath, + languageCode, + }) +} diff --git a/middleware/index.js b/middleware/index.js index c735e3ab06df..4f8281b51ed7 100644 --- a/middleware/index.js +++ b/middleware/index.js @@ -222,7 +222,7 @@ export default function (app) { app.use(instrument(handleRedirects, './redirects/handle-redirects')) // Must come before contextualizers // *** Config and context for rendering *** - app.use(instrument(findPage, './find-page')) // Must come before archived-enterprise-versions, breadcrumbs, featured-links, products, render-page + app.use(asyncMiddleware(instrument(findPage, './find-page'))) // Must come before archived-enterprise-versions, breadcrumbs, featured-links, products, render-page app.use(instrument(blockRobots, './block-robots')) // Check for a dropped connection before proceeding diff --git a/nodemon.json b/nodemon.json index aca75283315a..e5c16a7537db 100644 --- a/nodemon.json +++ b/nodemon.json @@ -5,6 +5,7 @@ "script", "translations", "stylesheets", - "tests" + "tests", + "content" ] } diff --git a/tests/unit/find-page-middleware.js b/tests/unit/find-page-middleware.js new file mode 100644 index 000000000000..98ca5a1eecfa --- /dev/null +++ b/tests/unit/find-page-middleware.js @@ -0,0 +1,89 @@ +import { fileURLToPath } from 'url' +import path from 'path' +import http from 'http' + +import { expect, describe, test } from '@jest/globals' + +import Page from '../../lib/page.js' +import findPage from '../../middleware/find-page.js' + +const __dirname = path.dirname(fileURLToPath(import.meta.url)) + +function makeRequestResponse(url, currentVersion = 'free-pro-team@latest') { + const req = new http.IncomingMessage(null) + req.method = 'GET' + req.url = url + req.path = url + req.cookies = {} + req.headers = {} + + // Custom keys on the request + req.pagePath = url + req.context = {} + req.context.currentVersion = currentVersion + req.context.pages = {} + + return [req, new http.ServerResponse(req)] +} + +describe('find page middleware', () => { + test('attaches page on req.context', async () => { + const url = '/en/foo/bar' + const [req, res] = makeRequestResponse(url) + req.context.pages = { + '/en/foo/bar': await Page.init({ + relativePath: 'page-with-redirects.md', + basePath: path.join(__dirname, '../fixtures'), + languageCode: 'en', + }), + } + + let nextCalls = 0 + await findPage(req, res, () => { + nextCalls++ + }) + expect(nextCalls).toBe(1) + expect(req.context.page).toBeInstanceOf(Page) + }) + + test('does not attach page on req.context if not found', async () => { + const [req, res] = makeRequestResponse('/en/foo/bar') + + let nextCalls = 0 + await findPage(req, res, () => { + nextCalls++ + }) + expect(nextCalls).toBe(1) + expect(req.context.page).toBe(undefined) + }) + + test('re-reads from disk if in development mode', async () => { + const [req, res] = makeRequestResponse('/en/page-with-redirects') + + await findPage(req, res, () => {}, { + isDev: true, + contentRoot: path.join(__dirname, '../fixtures'), + }) + expect(req.context.page).toBeInstanceOf(Page) + }) + + test('finds it for non-fpt version URLS', async () => { + const [req, res] = makeRequestResponse('/en/page-with-redirects', 'enterprise-cloud@latest') + + await findPage(req, res, () => {}, { + isDev: true, + contentRoot: path.join(__dirname, '../fixtures'), + }) + expect(req.context.page).toBeInstanceOf(Page) + }) + + test('re-reads from disk if in development mode and finds nothing', async () => { + const [req, res] = makeRequestResponse('/en/never/heard/of') + + await findPage(req, res, () => {}, { + isDev: true, + contentRoot: path.join(__dirname, '../fixtures'), + }) + expect(req.context.page).toBe(undefined) + }) +})