From 64fe173a17f0a7e2050a50e070d26218df0a89ad Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Tue, 23 Nov 2021 11:47:07 +0000 Subject: [PATCH 1/2] fix: don't move files to the CDN if they match redirects or beforeFiles rewrites --- demo/next.config.js | 8 ++--- demo/pages/old/image.js | 7 ++++ demo/pages/redirectme.js | 7 ++++ src/helpers/files.js | 76 ++++++++++++++++++++++++++++++++++++++-- 4 files changed, 92 insertions(+), 6 deletions(-) create mode 100644 demo/pages/old/image.js create mode 100644 demo/pages/redirectme.js diff --git a/demo/next.config.js b/demo/next.config.js index 93c2cbab23..59274ee24d 100644 --- a/demo/next.config.js +++ b/demo/next.config.js @@ -4,7 +4,7 @@ module.exports = { generateBuildId: () => 'build-id', i18n: { defaultLocale: 'en', - locales: ['en', 'es', 'fr'] + locales: ['en', 'es', 'fr'], }, async headers() { return [ @@ -14,7 +14,7 @@ module.exports = { { key: 'x-custom-header', value: 'my custom header value', - } + }, ], }, ] @@ -29,8 +29,8 @@ module.exports = { { source: '/old/:path*', destination: '/:path*', - } - ] + }, + ], } }, // Redirects allow you to redirect an incoming request path to a different destination path. diff --git a/demo/pages/old/image.js b/demo/pages/old/image.js new file mode 100644 index 0000000000..d438a8bea1 --- /dev/null +++ b/demo/pages/old/image.js @@ -0,0 +1,7 @@ +const Images = () => ( +
+

If you can see this, rewrites aren't working

+
+) + +export default Images diff --git a/demo/pages/redirectme.js b/demo/pages/redirectme.js new file mode 100644 index 0000000000..51105a1aa3 --- /dev/null +++ b/demo/pages/redirectme.js @@ -0,0 +1,7 @@ +const Redirect = () => ( +
+

If you can see this, redirects aren't working

+
+) + +export default Redirect diff --git a/src/helpers/files.js b/src/helpers/files.js index 2e4ce6febf..0c73e566e5 100644 --- a/src/helpers/files.js +++ b/src/helpers/files.js @@ -1,4 +1,4 @@ -// @ts-check +/* eslint-disable max-lines */ const { cpus } = require('os') const { yellowBright } = require('chalk') @@ -28,10 +28,33 @@ const matchMiddleware = (middleware, filePath) => filePath === middlewarePath || filePath === `${middlewarePath}.html` || filePath.startsWith(`${middlewarePath}/`), ) +const matchesRedirect = (file, redirects) => { + if (!Array.isArray(redirects)) { + return false + } + return redirects.some((redirect) => { + if (!redirect.regex || redirect.internal) { + return false + } + // Strips the extension from the file path + return new RegExp(redirect.regex).test(`/${file.slice(0, -5)}`) + }) +} + +const matchesRewrite = (file, rewrites) => { + if (!Array.isArray(rewrites?.beforeFiles)) { + return false + } + return matchesRedirect(file, rewrites.beforeFiles) +} + +exports.matchesRedirect = matchesRedirect +exports.matchesRewrite = matchesRewrite exports.matchMiddleware = matchMiddleware exports.stripLocale = stripLocale exports.isDynamicRoute = isDynamicRoute +// eslint-disable-next-line max-lines-per-function exports.moveStaticPages = async ({ netlifyConfig, target, i18n }) => { console.log('Moving static page files to serve from CDN...') const outputDir = join(netlifyConfig.build.publish, target === 'server' ? 'server' : 'serverless') @@ -48,6 +71,7 @@ exports.moveStaticPages = async ({ netlifyConfig, target, i18n }) => { } const prerenderManifest = await readJson(join(netlifyConfig.build.publish, 'prerender-manifest.json')) + const { redirects, rewrites } = await readJson(join(netlifyConfig.build.publish, 'routes-manifest.json')) const isrFiles = new Set() @@ -79,6 +103,8 @@ exports.moveStaticPages = async ({ netlifyConfig, target, i18n }) => { const matchingMiddleware = new Set() const matchedPages = new Set() + const matchedRedirects = new Set() + const matchedRewrites = new Set() // Limit concurrent file moves to number of cpus or 2 if there is only 1 const limit = pLimit(Math.max(2, cpus().length)) @@ -91,6 +117,14 @@ exports.moveStaticPages = async ({ netlifyConfig, target, i18n }) => { if (isDynamicRoute(filePath)) { return } + if (matchesRedirect(filePath, redirects)) { + matchedRedirects.add(filePath) + return + } + if (matchesRewrite(filePath, rewrites)) { + matchedRewrites.add(filePath) + return + } // Middleware matches against the unlocalised path const unlocalizedPath = stripLocale(rawPath, i18n?.locales) const middlewarePath = matchMiddleware(middleware, unlocalizedPath) @@ -110,7 +144,8 @@ exports.moveStaticPages = async ({ netlifyConfig, target, i18n }) => { yellowBright(outdent` Skipped moving ${matchedPages.size} ${ matchedPages.size === 1 ? 'file because it matches' : 'files because they match' - } middleware, so cannot be deployed to the CDN and will be served from the origin instead. This is fine, but we're letting you know because it may not be what you expect. + } middleware, so cannot be deployed to the CDN and will be served from the origin instead. + This is fine, but we're letting you know because it may not be what you expect. `), ) @@ -119,6 +154,8 @@ exports.moveStaticPages = async ({ netlifyConfig, target, i18n }) => { The following middleware matched statically-rendered pages: ${yellowBright([...matchingMiddleware].map((mid) => `- /${mid}/_middleware`).join('\n'))} + + ──────────────────────────────────────────────────────────────── `, ) // There could potentially be thousands of matching pages, so we don't want to spam the console with this @@ -128,6 +165,40 @@ exports.moveStaticPages = async ({ netlifyConfig, target, i18n }) => { The following files matched middleware and were not moved to the CDN: ${yellowBright([...matchedPages].map((mid) => `- ${mid}`).join('\n'))} + + ──────────────────────────────────────────────────────────────── + `, + ) + } + } + + if (matchedRedirects.size !== 0 || matchedRewrites.size !== 0) { + console.log( + yellowBright(outdent` + Skipped moving ${ + matchedRedirects.size + matchedRewrites.size + } files because they match redirects or beforeFiles rewrites, so cannot be deployed to the CDN and will be served from the origin instead. + `), + ) + if (matchedRedirects.size < 50 && matchedRedirects.size !== 0) { + console.log( + outdent` + The following files matched redirects and were not moved to the CDN: + + ${yellowBright([...matchedRedirects].map((mid) => `- ${mid}`).join('\n'))} + + ──────────────────────────────────────────────────────────────── + `, + ) + } + if (matchedRewrites.size < 50 && matchedRewrites.size !== 0) { + console.log( + outdent` + The following files matched beforeFiles rewrites and were not moved to the CDN: + + ${yellowBright([...matchedRewrites].map((mid) => `- ${mid}`).join('\n'))} + + ──────────────────────────────────────────────────────────────── `, ) } @@ -151,3 +222,4 @@ exports.movePublicFiles = async ({ appDir, publish }) => { await copy(publicDir, `${publish}/`) } } +/* eslint-enable max-lines */ From aa17745dbf1e75167dbae7590e5ba2e4f516f100 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Tue, 23 Nov 2021 13:36:24 +0000 Subject: [PATCH 2/2] chore: add tests Release-As: 4.0.0-beta.10 --- src/helpers/files.js | 3 ++ test/index.js | 81 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/helpers/files.js b/src/helpers/files.js index 0c73e566e5..62eed1dfaa 100644 --- a/src/helpers/files.js +++ b/src/helpers/files.js @@ -42,6 +42,9 @@ const matchesRedirect = (file, redirects) => { } const matchesRewrite = (file, rewrites) => { + if (Array.isArray(rewrites)) { + return matchesRedirect(file, rewrites) + } if (!Array.isArray(rewrites?.beforeFiles)) { return false } diff --git a/test/index.js b/test/index.js index e8cfcb0532..2e391244a8 100644 --- a/test/index.js +++ b/test/index.js @@ -9,7 +9,7 @@ const plugin = require('../src') const { HANDLER_FUNCTION_NAME, ODB_FUNCTION_NAME } = require('../src/constants') const { join } = require('pathe') -const { matchMiddleware, stripLocale } = require('../src/helpers/files') +const { matchMiddleware, stripLocale, matchesRedirect, matchesRewrite } = require('../src/helpers/files') const FIXTURES_DIR = `${__dirname}/fixtures` const SAMPLE_PROJECT_DIR = `${__dirname}/../demo` @@ -31,6 +31,45 @@ const utils = { }, } +const REDIRECTS = [ + { + source: '/:file((?!\\.well-known(?:/.*)?)(?:[^/]+/)*[^/]+\\.\\w+)/', + destination: '/:file', + locale: false, + internal: true, + statusCode: 308, + regex: '^(?:/((?!\\.well-known(?:/.*)?)(?:[^/]+/)*[^/]+\\.\\w+))/$', + }, + { + source: '/:notfile((?!\\.well-known(?:/.*)?)(?:[^/]+/)*[^/\\.]+)', + destination: '/:notfile/', + locale: false, + internal: true, + statusCode: 308, + regex: '^(?:/((?!\\.well-known(?:/.*)?)(?:[^/]+/)*[^/\\.]+))$', + }, + { + source: '/en/redirectme', + destination: '/', + statusCode: 308, + regex: '^(?!/_next)/en/redirectme(?:/)?$', + }, + { + source: '/:nextInternalLocale(en|es|fr)/redirectme', + destination: '/:nextInternalLocale/', + statusCode: 308, + regex: '^(?!/_next)(?:/(en|es|fr))/redirectme(?:/)?$', + }, +] + +const REWRITES = [ + { + source: '/:nextInternalLocale(en|es|fr)/old/:path*', + destination: '/:nextInternalLocale/:path*', + regex: '^(?:/(en|es|fr))/old(?:/((?:[^/]+?)(?:/(?:[^/]+?))*))?(?:/)?$', + }, +] + // Temporary switch cwd const changeCwd = function (cwd) { const originalCwd = process.cwd() @@ -447,4 +486,44 @@ describe('utility functions', () => { expect(stripLocale(path, locales)).toEqual(path) } }) + + test('matchesRedirect correctly matches paths with locales', () => { + const paths = ['en/redirectme.html', 'en/redirectme.json', 'fr/redirectme.html', 'fr/redirectme.json'] + paths.forEach((path) => { + expect(matchesRedirect(path, REDIRECTS)).toBeTruthy() + }) + }) + + test("matchesRedirect doesn't match paths with invalid locales", () => { + const paths = ['dk/redirectme.html', 'dk/redirectme.json', 'gr/redirectme.html', 'gr/redirectme.json'] + paths.forEach((path) => { + expect(matchesRedirect(path, REDIRECTS)).toBeFalsy() + }) + }) + + test("matchesRedirect doesn't match internal redirects", () => { + const paths = ['en/notrailingslash'] + paths.forEach((path) => { + expect(matchesRedirect(path, REDIRECTS)).toBeFalsy() + }) + }) + + it('matchesRewrite matches array of rewrites', () => { + expect(matchesRewrite('en/old/page.html', REWRITES)).toBeTruthy() + }) + + it('matchesRewrite matches beforeFiles rewrites', () => { + expect(matchesRewrite('en/old/page.html', { beforeFiles: REWRITES })).toBeTruthy() + }) + + it("matchesRewrite doesn't match afterFiles rewrites", () => { + expect(matchesRewrite('en/old/page.html', { afterFiles: REWRITES })).toBeFalsy() + }) + + it('matchesRewrite matches various paths', () => { + const paths = ['en/old/page.html', 'fr/old/page.html', 'en/old/deep/page.html', 'en/old.html'] + paths.forEach((path) => { + expect(matchesRewrite(path, REWRITES)).toBeTruthy() + }) + }) })