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.

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

Removes store caching during SSR #307

Merged
merged 2 commits into from
Dec 23, 2024
Merged

Removes store caching during SSR #307

merged 2 commits into from
Dec 23, 2024

Conversation

joshnuss
Copy link
Owner

This PR removes caching of stores during Server Side Rendering (SSR)

Problem

Caching stores server side makes state leak between requests, since the global state is shared between requests

Solution

Avoid using global state during SSR (when window is undefined)

Caveats

If the a store with the same key is instantiated twice, it creates two seperate stores.

const store1 = persisted('my-key', initial)
const store2 = persisted('my-key', initial)

// during SSR
// store1 != store2, because their is no global cache of stores

// during CSR
// store1 == store2 because their is a global cache

Closes #306

const store = internal(initial, (set) => {
if (browser && storageType == 'local' && syncTabs) {
const handleStorage = (event: StorageEvent) => {
if (event.key === key && event.newValue) {
Copy link

Choose a reason for hiding this comment

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

If event.newValue is falsy, are you not interpreting that as a value deletion? I don't remember if this NPM package deletes values or not.

Copy link
Owner Author

Choose a reason for hiding this comment

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

event.newValue is a JSON string, so if it's falsey, it's because it's undefined.
If memory serves, it's undefined when the key was deleted in another tab.

@webJose
Copy link

webJose commented Dec 22, 2024

I just had a question. I saw nothing obviously wrong or anything. On the contrary, it looks good and adding the extra unit tests should increase the confidence on the changes.

@joshnuss joshnuss merged commit 4cafa41 into master Dec 23, 2024
4 checks passed
@joshnuss joshnuss deleted the remove-cache-ssr branch December 23, 2024 02:57
@joshnuss
Copy link
Owner Author

All good,
And thanks for the review @webJose!
Extra set of eyes is much appreciated 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

State leakage between users is possible using this module
2 participants