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
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 |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 changeset
Testing
Docs
I do not think docs are required as this is a patch for the existing functionality?