Content Collection cache (experimental)#8854
Merged
natemoo-re merged 62 commits intomainfrom Nov 9, 2023
Merged
Conversation
🦋 Changeset detectedLatest commit: 3d61b49 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
natemoo-re
commented
Oct 17, 2023
af787e4 to
ba83398
Compare
6636df2 to
aba219d
Compare
Member
Author
|
!preview content-cache |
Contributor
|
f38cdb9 to
b1f127e
Compare
Member
Author
|
!preview content-cache |
Contributor
|
f97ffa7 to
7870d0c
Compare
Member
Author
|
Working on a refactor to be stacked against this PR refactor/content-collections...refactor/collections-build |
7b52b92 to
5466fb6
Compare
Member
Author
|
/preview content-cache |
Contributor
|
Snapshots have been released for the following packages:
Publish LogBuild Log |
84dd3a2 to
b14a2bc
Compare
Member
Author
|
/preview content-cache |
Contributor
|
Snapshots have been released for the following packages:
Publish LogBuild Log |
sarah11918
approved these changes
Nov 8, 2023
Member
sarah11918
left a comment
There was a problem hiding this comment.
Looks great! Happy to see this one squeak in! 🙌 Tiny suggestion for your consideration below!
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
matthewp
reviewed
Nov 8, 2023
matthewp
reviewed
Nov 8, 2023
matthewp
reviewed
Nov 8, 2023
matthewp
reviewed
Nov 8, 2023
matthewp
reviewed
Nov 8, 2023
matthewp
reviewed
Nov 8, 2023
matthewp
approved these changes
Nov 9, 2023
Merged
ematipico
reviewed
Nov 9, 2023
Comment on lines
+42
to
+44
| configResolved(config) { | ||
| IS_DEV = config.mode === 'development' | ||
| }, |
Member
There was a problem hiding this comment.
settings already contains the config; why do we need configResolved in this case?
| return; | ||
| } | ||
| if (code.includes(RESOLVED_VIRTUAL_MODULE_ID)) { | ||
| const depth = chunk.fileName.split('/').length - 1; |
| function getStringifiedCollectionFromLookup(wantedType: 'content' | 'data' | 'render', relContentDir: string, lookupMap: ContentLookupMap) { | ||
| let str = '{'; | ||
| // In dev, we don't need to normalize the import specifier at all. Vite handles it. | ||
| let normalize = (slug: string) => slug; |
Member
There was a problem hiding this comment.
This can be let normalize because if/else both reach the point where we do normalize =
| let suffix = ''; | ||
| if (wantedType === 'content') suffix = CONTENT_FLAG; | ||
| else if (wantedType === 'data') suffix = DATA_FLAG; | ||
| else if (wantedType === 'render') suffix = CONTENT_RENDER_FLAG; |
Member
There was a problem hiding this comment.
This can be else because we checked all types
| `lookupMap = ${stringifiedLookupMap};` | ||
| ), | ||
| }; | ||
| function getStringifiedCollectionFromLookup(wantedType: 'content' | 'data' | 'render', relContentDir: string, lookupMap: ContentLookupMap) { |
Member
There was a problem hiding this comment.
It seems we introduced some new logic: 'content' | 'data' | 'render'. Maybe it's worth having a comment that explains what we do.
If you think it's too technical, we could also have a README.md
Comment on lines
+180
to
+191
| await fsMod.promises.mkdir(contentCacheDir, { recursive: true }); | ||
| await fsMod.promises.writeFile(contentManifestFile, JSON.stringify(newManifest), { encoding: 'utf8' }); | ||
|
|
||
| const cacheExists = fsMod.existsSync(cache); | ||
| fsMod.mkdirSync(cache, { recursive: true }) | ||
| await fsMod.promises.mkdir(cacheTmp, { recursive: true }); | ||
| await copyFiles(distContentRoot, cacheTmp, true); | ||
| if (cacheExists) { | ||
| await copyFiles(contentCacheDir, distContentRoot, false); | ||
| } | ||
| await copyFiles(cacheTmp, contentCacheDir); | ||
| await fsMod.promises.rm(cacheTmp, { recursive: true, force: true }); |
Member
There was a problem hiding this comment.
Can we enclose all these FS operations in a try/catch block? There might be remote situations where we don't have write access
| ]); | ||
| newManifest.serverEntries = Array.from(serverComponents); | ||
| newManifest.clientEntries = Array.from(clientComponents); | ||
| await fsMod.promises.mkdir(contentCacheDir, { recursive: true }); |
Member
There was a problem hiding this comment.
Should we check if contentCacheDir exists already?
|
|
||
| async function generateContentManifest(opts: StaticBuildOptions, lookupMap: ContentLookupMap): Promise<ContentManifest> { | ||
| let manifest: ContentManifest = { version: CONTENT_MANIFEST_VERSION, entries: [], serverEntries: [], clientEntries: [] }; | ||
| const limit = pLimit(10); |
| const key: ContentManifestKey = { collection, type, entry }; | ||
| const fileURL = new URL(encodeURI(joinPaths(opts.settings.config.root.toString(), entry))); | ||
| promises.push(limit(async () => { | ||
| const data = await fsMod.promises.readFile(fileURL, { encoding: 'utf8' }); |
Member
There was a problem hiding this comment.
Is this operation safe? Should we check if fileURL exists and if we can read the file?
| return false; | ||
| } | ||
|
|
||
| export function encodeName(name: string): string { |
Member
There was a problem hiding this comment.
We should at least explain what kind of encoding this function does. Or maybe an example.
Merged
natemoo-re
added a commit
that referenced
this pull request
Nov 22, 2023
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca> Co-authored-by: Matthew Phillips <matthew@skypack.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
experimental.contentCollectionCacheflag. This flag changes a few things about our build process, which allows us to share Content Collection assets between the build.astro build --forceflag to clear the cache if needed.import.meta.glob, which will invalidate a collection every time one of the modules it references changes, this implements a custom build for content collections where individual modules can be built separately.In order to try this PR out locally, run:
Real-world results from this PR:
Note that the build still renders every page, which can take a significant time with large projects.
2.5K items
Uncached 33.31s
Cached 10.53s
5K items
Uncached 86.70s
Cached 44.17s
Docs Repo (Real World)
Uncached 133.20s
Cached 10.46s
Testing
Added a few initial test suites for the experimental behavior. To remove the experimental flag, we'll need more comprehensive test coverage.
Docs
For review:
.changeset/lovely-pianos-build.mdpackages/astro/src/@types/astro.ts