Skip to content

fix: don't deploy metadata files to CDN #2104

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.

Sign up for GitHub

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 21 commits into from
May 19, 2023
Merged

fix: don't deploy metadata files to CDN #2104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
199a442
fix: tRaCe
pieh May 11, 2023
02b19ab
chore: add more metadata files for deletion
pieh May 17, 2023
7309968
refactor: move concrete file removal logic into it's own function
pieh May 17, 2023
271e5fe
chore: make hidden paths consistent
pieh May 17, 2023
73a757d
test: update snapshot and add unit test for file removal
pieh May 17, 2023
f24dc38
chore: add code comment about timing of metadata files removal
pieh May 17, 2023
3c0147f
fix: publish 404 and 500 to cdn
pieh May 17, 2023
0566f2e
test: don't delete BUILD_ID in test env
pieh May 17, 2023
68f699a
test: update snapshot
pieh May 17, 2023
bee62a1
fix: typescript gods
pieh May 17, 2023
c94a81c
chore: dummy commit
pieh May 17, 2023
8f28acd
chore: dummy commit2
pieh May 17, 2023
c303d75
chore: dummy commit3
pieh May 17, 2023
d228aed
test: update snapshot
pieh May 17, 2023
3e9080d
test: use different dedicated env var to skip deleting BUILD_ID
pieh May 17, 2023
0fad9fc
test: update utils.spec.ts assertions
pieh May 17, 2023
1bef70c
test: keep all the metadata for e2e tests
pieh May 17, 2023
465bcda
chore: update comment
pieh May 17, 2023
d709eb1
perf: delete files concurrently instead of 1 by 1
pieh May 18, 2023
3399455
chore: update comment
pieh May 18, 2023
a9b22fc
Merge branch 'main' into tRaCe
kodiakhq[bot] May 19, 2023
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
37 changes: 25 additions & 12 deletions packages/runtime/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import destr from 'destr'

export const HANDLER_FUNCTION_NAME = '___netlify-handler'
export const ODB_FUNCTION_NAME = '___netlify-odb-handler'
export const IMAGE_FUNCTION_NAME = '_ipx'
Expand All @@ -7,18 +9,29 @@ export const HANDLER_FUNCTION_TITLE = 'Next.js SSR handler'
export const ODB_FUNCTION_TITLE = 'Next.js ISR handler'
export const IMAGE_FUNCTION_TITLE = 'next/image handler'
// These are paths in .next that shouldn't be publicly accessible
export const HIDDEN_PATHS = [
'/cache/*',
'/server/*',
'/serverless/*',
'/trace',
'/traces',
'/routes-manifest.json',
'/build-manifest.json',
'/prerender-manifest.json',
'/react-loadable-manifest.json',
'/BUILD_ID',
]
export const HIDDEN_PATHS = destr(process.env.NEXT_KEEP_METADATA_FILES)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is primarily for test/e2e as those try to check metadata files after deployment, demos + cypress tests will delete those files on the other hand

Copy link

Choose a reason for hiding this comment

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

Since it's for E2E/CI only (primarily), we already have a CI environment variable. See a usage of it here, https://github.com/netlify/next-runtime/blob/82c1ea371bbe2194058264d7cfad912343334809/package.json#L19.

Suggested change
export const HIDDEN_PATHS = destr(process.env.NEXT_KEEP_METADATA_FILES)
export const HIDDEN_PATHS = destr(process.env.CI)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CI env var is true on Netlify platform, so we can't rely on that, otherwise we won't actually delete those meta files :(

? []
: [
'/cache',
'/server',
'/serverless',
'/trace',
'/traces',
'/routes-manifest.json',
'/build-manifest.json',
'/prerender-manifest.json',
'/react-loadable-manifest.json',
'/BUILD_ID',
'/app-build-manifest.json',
'/app-path-routes-manifest.json',
'/export-marker.json',
'/images-manifest.json',
'/next-server.js.nft.json',
'/package.json',
'/prerender-manifest.js',
'/required-server-files.json',
'/static-manifest.json',
]

export const ODB_FUNCTION_PATH = `/.netlify/builders/${ODB_FUNCTION_NAME}`
export const HANDLER_FUNCTION_PATH = `/.netlify/functions/${HANDLER_FUNCTION_NAME}`
Expand Down
31 changes: 27 additions & 4 deletions packages/runtime/src/helpers/files.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,26 @@ import { cpus } from 'os'

import type { NetlifyConfig } from '@netlify/build'
import { yellowBright } from 'chalk'
import { existsSync, readJson, move, copy, writeJson, readFile, writeFile, ensureDir, readFileSync } from 'fs-extra'
import {
existsSync,
readJson,
move,
copy,
writeJson,
readFile,
writeFile,
ensureDir,
readFileSync,
remove,
} from 'fs-extra'
import globby from 'globby'
import { PrerenderManifest } from 'next/dist/build'
import { outdent } from 'outdent'
import pLimit from 'p-limit'
import { join, resolve, dirname } from 'pathe'
import slash from 'slash'

import { MINIMUM_REVALIDATE_SECONDS, DIVIDER } from '../constants'
import { MINIMUM_REVALIDATE_SECONDS, DIVIDER, HIDDEN_PATHS } from '../constants'

import { NextConfig } from './config'
import { loadPrerenderManifest } from './edge'
Expand Down Expand Up @@ -138,8 +149,8 @@ export const moveStaticPages = async ({
console.warn('Error moving file', source, error)
}
}
// Move all static files, except error documents and nft manifests
const pages = await globby(['{app,pages}/**/*.{html,json,rsc}', '!**/(500|404|*.js.nft).{html,json}'], {
// Move all static files, except nft manifests
const pages = await globby(['{app,pages}/**/*.{html,json,rsc}', '!**/*.js.nft.{html,json}'], {
cwd: outputDir,
dot: true,
})
Expand Down Expand Up @@ -467,3 +478,15 @@ export const movePublicFiles = async ({
await copy(publicDir, `${publish}${basePath}/`)
}
}

export const removeMetadataFiles = async (publish: string) => {
Copy link

Choose a reason for hiding this comment

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

Would it not be faster to await Promise.all() or await Promise.allSettled()?

Copy link
Contributor Author

@pieh pieh May 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Generally yes, but if we don't want to do 1 by 1 for perf reasons we should still limit concurrency as otherwise some starvation might happen and doing just Promise.all() might actually be slower than doing it 1 by 1

For files moving we do have concurency of number of cpu cores, with minimum of 2 (if on single core):

https://github.com/netlify/next-runtime/blob/f051e227ca10ddc228b1d044eb835fe66fd1d733/packages/runtime/src/helpers/files.ts#L152-L153

So I would suggest we do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Swapped to p-limit (like snippet above) in d709eb1

// Limit concurrent deletions to number of cpus or 2 if there is only 1
const limit = pLimit(Math.max(2, cpus().length))

const removePromises = HIDDEN_PATHS.map((HIDDEN_PATH) => {
const pathToDelete = join(publish, HIDDEN_PATH)
return limit(() => remove(pathToDelete))
})

await Promise.all(removePromises)
}
12 changes: 1 addition & 11 deletions packages/runtime/src/helpers/redirects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type { PrerenderManifest, SsgRoute } from 'next/dist/build'
import { outdent } from 'outdent'
import { join } from 'pathe'

import { HANDLER_FUNCTION_PATH, HIDDEN_PATHS, ODB_FUNCTION_PATH } from '../constants'
import { HANDLER_FUNCTION_PATH, ODB_FUNCTION_PATH } from '../constants'

import { isAppDirRoute, loadAppPathRoutesManifest } from './edge'
import { getMiddleware } from './files'
Expand All @@ -27,14 +27,6 @@ import {
const matchesMiddleware = (middleware: Array<string>, route: string): boolean =>
middleware.some((middlewarePath) => route.startsWith(middlewarePath))

const generateHiddenPathRedirects = ({ basePath }: Pick<NextConfig, 'basePath'>): NetlifyConfig['redirects'] =>
HIDDEN_PATHS.map((path) => ({
from: `${basePath}${path}`,
to: '/404.html',
status: 404,
force: true,
}))

const generateLocaleRedirects = ({
i18n,
basePath,
Expand Down Expand Up @@ -281,8 +273,6 @@ export const generateRedirects = async ({
join(netlifyConfig.build.publish, 'routes-manifest.json'),
)

netlifyConfig.redirects.push(...generateHiddenPathRedirects({ basePath }))

if (i18n && i18n.localeDetection !== false) {
netlifyConfig.redirects.push(...generateLocaleRedirects({ i18n, basePath, trailingSlash }))
}
Expand Down
2 changes: 1 addition & 1 deletion packages/runtime/src/helpers/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ export const redirectsForNext404Route = ({
}): NetlifyConfig['redirects'] =>
netlifyRoutesForNextRoute({ route, buildId, i18n }).map(({ redirect, locale }) => ({
from: `${basePath}${redirect}`,
to: locale ? `${basePath}/server/pages/${locale}/404.html` : `${basePath}/server/pages/404.html`,
to: locale ? `${basePath}/${locale}/404.html` : `${basePath}/404.html`,
status: 404,
force,
}))
Expand Down
8 changes: 7 additions & 1 deletion packages/runtime/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
} from './helpers/config'
import { onPreDev } from './helpers/dev'
import { writeEdgeFunctions, loadMiddlewareManifest, cleanupEdgeFunctions } from './helpers/edge'
import { moveStaticPages, movePublicFiles, patchNextFiles } from './helpers/files'
import { moveStaticPages, movePublicFiles, patchNextFiles, removeMetadataFiles } from './helpers/files'
import { splitApiRoutes } from './helpers/flags'
import {
generateFunctions,
Expand Down Expand Up @@ -254,6 +254,12 @@ const plugin: NetlifyPlugin = {
warnForProblematicUserRewrites({ basePath, redirects })
warnForRootRedirects({ appDir })
await warnOnApiRoutes({ FUNCTIONS_DIST })

// we are removing metadata files from Publish directory
// we have to do this after functions were bundled as bundling still
// require those files, but we don't want to publish them
await removeMetadataFiles(publish)

if (experimental?.appDir) {
console.log(
'🧪 Thank you for testing "appDir" support on Netlify. For known issues and to give feedback, visit https://ntl.fyi/next-13-feedback',
Expand Down
Loading