Skip to content
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

Jump to bottom

Make the incremental cache storage and cache providers extensible. #22619

Closed
wants to merge 1 commit into from
Closed

Make the incremental cache storage and cache providers extensible. #22619

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
71 changes: 44 additions & 27 deletions packages/next/next-server/server/incremental-cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,19 @@
import { promises, readFileSync } from 'fs'
import { readFileSync, promises } from 'fs'
import LRUCache from 'next/dist/compiled/lru-cache'
import path from 'path'
import { PrerenderManifest } from '../../build'
import { PRERENDER_MANIFEST } from '../lib/constants'
import {
FileSystemStorageProvider,
StorageProfiderInterface,
} from './lib/storage'
import { normalizePagePath } from './normalize-page-path'

function toRoute(pathname: string): string {
return pathname.replace(/\/$/, '').replace(/\/index$/, '') || '/'
}

type IncrementalCacheValue = {
export type IncrementalCacheValue = {
html?: string
pageData?: any
isStale?: boolean
Expand All @@ -31,6 +35,7 @@ export class IncrementalCache {
prerenderManifest: PrerenderManifest
cache: LRUCache<string, IncrementalCacheValue>
locales?: string[]
storageProvider: StorageProfiderInterface
Copy link

Choose a reason for hiding this comment

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

typo


constructor({
max,
Expand All @@ -39,13 +44,17 @@ export class IncrementalCache {
pagesDir,
flushToDisk,
locales,
storageProvider,
lruCacheProvider,
}: {
dev: boolean
max?: number
distDir: string
pagesDir: string
flushToDisk?: boolean
locales?: string[]
storageProvider?: StorageProfiderInterface
Copy link

Choose a reason for hiding this comment

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

Instead of passing this in here, could it be an implementation detail of the lruCacheProvider?

In other words, could the default lruCacheProvider be built of both an in-memory LRUCache and a FilesystemCache fallback?

That would require moving some logic out of this file into something like a memory-cache-with-disk-fallback.ts. The benefit is that it would simplify this class's interface a bit.

lruCacheProvider?: LRUCache<string, IncrementalCacheValue>
}) {
this.incrementalOptions = {
dev,
Expand All @@ -70,15 +79,19 @@ export class IncrementalCache {
)
}

this.cache = new LRUCache({
// default to 50MB limit
max: max || 50 * 1024 * 1024,
length(val) {
if (val.isNotFound || val.isRedirect) return 25
// rough estimate of size of cache value
return val.html!.length + JSON.stringify(val.pageData).length
},
})
this.storageProvider = storageProvider || new FileSystemStorageProvider()

this.cache =
lruCacheProvider ||
new LRUCache({
// default to 50MB limit
max: max || 50 * 1024 * 1024,
length(val) {
if (val.isNotFound || val.isRedirect) return 25
// rough estimate of size of cache value
return val.html!.length + JSON.stringify(val.pageData).length
},
})
}

private getSeedPath(pathname: string, ext: string): string {
Expand Down Expand Up @@ -116,31 +129,34 @@ export class IncrementalCache {
if (this.incrementalOptions.dev) return
pathname = normalizePagePath(pathname)

let data = this.cache.get(pathname)
// Await in case the actual cache implementation returns a promise.
let data = await this.cache.get(pathname)

// let's check the disk for seed data
// let's check the storage provider for seed data
if (!data) {
if (this.prerenderManifest.notFoundRoutes.includes(pathname)) {
return { isNotFound: true, revalidateAfter: false }
}

try {
const html = await promises.readFile(
this.getSeedPath(pathname, 'html'),
'utf8'
const html = await this.storageProvider.readValue(
this.getSeedPath(pathname, 'html')
)
const pageData = JSON.parse(
await promises.readFile(this.getSeedPath(pathname, 'json'), 'utf8')
await this.storageProvider.readValue(
this.getSeedPath(pathname, 'json')
)
)

data = {
html,
pageData,
revalidateAfter: this.calculateRevalidate(pathname),
}
this.cache.set(pathname, data)
// Await in case the actual cache implementation returns a promise.
await this.cache.set(pathname, data)
} catch (_) {
// unable to get data from disk
// unable to get data from storage provider
}
}

Expand Down Expand Up @@ -187,7 +203,8 @@ export class IncrementalCache {
}

pathname = normalizePagePath(pathname)
this.cache.set(pathname, {
// Await in case the actual cache implementation returns a promise.
await this.cache.set(pathname, {
...data,
revalidateAfter: this.calculateRevalidate(pathname),
})
Expand All @@ -196,16 +213,16 @@ export class IncrementalCache {
// `next build` output's manifest.
if (this.incrementalOptions.flushToDisk && !data.isNotFound) {
try {
const seedPath = this.getSeedPath(pathname, 'html')
await promises.mkdir(path.dirname(seedPath), { recursive: true })
await promises.writeFile(seedPath, data.html, 'utf8')
await promises.writeFile(
await this.storageProvider.write(
this.getSeedPath(pathname, 'html'),
data.html
)
await this.storageProvider.write(
this.getSeedPath(pathname, 'json'),
JSON.stringify(data.pageData),
'utf8'
JSON.stringify(data.pageData)
)
} catch (error) {
// failed to flush to disk
// failed to flush to storage provider
console.warn('Failed to update prerender files for', pathname, error)
}
}
Expand Down
19 changes: 19 additions & 0 deletions packages/next/next-server/server/lib/storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { promises } from 'fs'

export interface StorageProfiderInterface {
readValue: (key: string) => Promise<string>
write: (key: string, value: string | undefined) => Promise<void>
}

export class FileSystemStorageProvider implements StorageProfiderInterface {
async readValue(key: string): Promise<string> {
return await promises.readFile(key, 'utf8')
}

async write(key: string, value: string | undefined) {
await promises.mkdir(key.substring(0, key.lastIndexOf('/')), {
Copy link

@reboxer reboxer Jul 13, 2021 •
edited

Choose a reason for hiding this comment

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

Testing this pull request and apparently this makes the directory to be an empty string and causing write() to fail each time it tries to save the files to disk.

Failed to update prerender files for /test [Error: ENOENT: no such file or directory, mkdir ''] {
  errno: -4058,
  code: 'ENOENT',
  syscall: 'mkdir',
  path: ''
}

The file should require path and the line should be path.dirname(key) to achieve the same result as the current implementation.

Also, mkdir could be put in a different method and called before trying to write the HTML and JSON, as it will be called twice for each page.

recursive: true,
})
await promises.writeFile(key, String(value), 'utf8')
}
}
2 changes: 2 additions & 0 deletions packages/next/next-server/server/next-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,8 @@ export default class Server {
),
locales: this.nextConfig.i18n?.locales,
flushToDisk: !minimalMode && this.nextConfig.experimental.sprFlushToDisk,
storageProvider: this.nextConfig.storageProvider,
lruCacheProvider: this.nextConfig.lruCacheProvider,
})

/**
Expand Down