Skip to content

fix: don't move files to the CDN if they match redirect/rewrite rules #832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 24, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions demo/next.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ module.exports = {
generateBuildId: () => 'build-id',
i18n: {
defaultLocale: 'en',
locales: ['en', 'es', 'fr']
locales: ['en', 'es', 'fr'],
},
async headers() {
return [
Expand All @@ -14,7 +14,7 @@ module.exports = {
{
key: 'x-custom-header',
value: 'my custom header value',
}
},
],
},
]
Expand All @@ -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.
Expand Down
7 changes: 7 additions & 0 deletions demo/pages/old/image.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const Images = () => (
<div>
<p>If you can see this, rewrites aren&apos;t working</p>
</div>
)

export default Images
7 changes: 7 additions & 0 deletions demo/pages/redirectme.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const Redirect = () => (
<div>
<p>If you can see this, redirects aren&apos;t working</p>
</div>
)

export default Redirect
79 changes: 77 additions & 2 deletions src/helpers/files.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @ts-check
/* eslint-disable max-lines */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[sand] We should probably avoid disabling eslint and separate this file into smaller ones moving forward

const { cpus } = require('os')

const { yellowBright } = require('chalk')
Expand Down Expand Up @@ -28,10 +28,36 @@ 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)) {
return matchesRedirect(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
Copy link

@tiffafoo tiffafoo Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[sand] Same for eslint

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')
Expand All @@ -48,6 +74,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()

Expand Down Expand Up @@ -79,6 +106,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))
Expand All @@ -91,6 +120,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)
Expand All @@ -110,7 +147,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.
`),
)

Expand All @@ -119,6 +157,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
Expand All @@ -128,6 +168,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'))}

────────────────────────────────────────────────────────────────
`,
)
}
Expand All @@ -151,3 +225,4 @@ exports.movePublicFiles = async ({ appDir, publish }) => {
await copy(publicDir, `${publish}/`)
}
}
/* eslint-enable max-lines */
81 changes: 80 additions & 1 deletion test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand All @@ -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()
Expand Down Expand Up @@ -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()
})
})
})