fix: Prod build collections cache can be modified #10709
Conversation
🦋 Changeset detectedLatest commit: 051f6a7 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 |
github-actions
bot
added
the
pkg: astro
rishi-raj-jain
changed the title
|
Do you think you can tackle #9306 as well? Seems similar |
|
I think this PR addresses that as well (already) as it's not returning a reference anymore. What do you think? |
|
Can we have a changeset? Also, can you update the template with the relevant information on how you tested the fix? |
| @@ -107,6 +104,8 @@ export function createGetCollection({ | |||
| ); | |||
| cacheEntriesByCollection.set(collection, entries); | |||
| } | |||
| // Always return a new instance so consumers can safely mutate it | |||
| entries = [...cacheEntriesByCollection.get(collection)!]; | |||
There was a problem hiding this comment.
This can be expensive for large collections. Given this is corner case, can we use Object.freeze when we set it instead? That would have similar effect (can't push, splice, etc) but without doing it on ever request.
There was a problem hiding this comment.
- Slice method https://measurethat.net/Benchmarks/Show/2667/0/arrayprototypeslice-vs-spread-operator#latest_results_block seems to be much faster.
- Can not freeze the cached collection as individual pages can still expect to sort the collection based on it's attributes as done in https://github.com/withastro/astro/packages/astro/test/content-collections-render.test.js fixture: https://github.com/withastro/astro/blob/main/packages/astro/test/fixtures/content/src/pages/sort-blog-collection.astro#L7.
There was a problem hiding this comment.
Can not freeze the cached collection as individual pages can still expect to sort the collection
Where did you read this? I just tried to use Object.freeze, and it works
|
@rishi-raj-jain do you still plan on working on this? |
|
Hey yeah, let me get on to this by tomorrow. |
|
I am unable to locate the tests for content collection, may you help me with that, @ematipico? |
|
There's a |
Yup, the tests already cover this as recorded in my observation at #10709 (comment) |
I am not sure I understand. We want to make sure that the original content collection isn't modified and that test case doesn't do that. |
I aimed to prevent modifications to the cached collection, but I think my understanding is not correct. I'm going to close this so that the issue doesn't end up being stalled due to me. |
fixes #10700
Changes
What does this change?
Be short and concise. Bullet points can help!
Before/after screenshots can help as well.
Don't forget a changeset!
pnpm exec changesetTesting
Docs
I do not think docs are required as this is a patch for the existing functionality?