Open PeppeL-G opened 7 months ago
This seemed like a really weird case when I first read the description, but having cloned the repo and played with the code, it is reproducible. To understand what is happening here, I believe it is important to understand two key things about this library:
I believe what is happening here is the following:
/
, creating store1
/page-2/show
, creating store2
/page-2
, deleting the reference to store2
in the application layer (and therefore unsubscribing from it), but keeping it alive in the library layer/
page, updating store1
and the reference in localStorage, but as you have unsubscribed from store2
, this doesn't recieve the localStorage event, and therefore its value doesn't change/page-2/show
, and since store2
has been saved in the library layer, you retrieve this from the library cache with the old value and subscribe to itNow, this here is a good catch, and one that requires a minor rework of the central structure of this library. I propose two fixes:
I would love to draft up a PR for this, but I just wanted to check in with you @joshnuss, as these are some pretty significant changes to the library
@bertmad3400 Thanks for the detailed analysis!
When creating two persisted stores in the application layer with the same key, it is not two different stores refereeing to the same localStorage key, but instead the same store
Correct. Since the persisted
store is essentially a wrapper over a localStorage
key, having many copies around seemed like a waste and possible consistency error.
Persisted stores are never removed What if we fixed that? Then @PeppeL-G's issue would be resolved. Plus, it would fix a potential memory leak for apps that create many temporary stores.
So I have done a lot of digging and experimentation, and have concluded two things: This is more complicated than I assumed, and I'm a dumbass who can't read the manual.
As it turns out, there is a really good reason for the internal store cache, but it isn't the one given by @joshnuss (which even though I fundamentally disagree with is another valid reason for it)
This (Storage Events) won't work on the same browsing context that is making the changes — it is really a way for other browsing contexts on the domain using the storage to sync any changes that are made. Browsing contexts on other domains can't access the same storage objects.
- MDN
The need for an internal cache structure is the fact that multiple stores using the same key will not automatically stay in sync on the same page, as the storage event won't propagate. While this has the unfortunate (and un-documented) side-effect that stores using the same key, but different settings will use the settings of whichever store was created first (which can be really confusing), but I see no way around this.
Secondly however, while my analysis was rather thorough it was completely wrong. Store1 and Store2 shouldn't stay in sync because of the storage events (that doesn't even work), but because they are a reference to the very same store. Why this doesn't work I have no clue, and after digging into this source code, related test environment details and web documentation for the last 4 hours, I'm completely toasted. I might have a look at it another day
The need for an internal cache structure is the fact that multiple stores using the same key will not automatically stay in sync on the same page, as the storage event won't propagate
Is it possible to subscribe to storage
event multiple times? Then each store could subscribe to the storage
event, and unsusbscribe during cleanup and we wouldn't need a global mapping of keys/stores.
Is it possible to subscribe to
storage
event multiple times? Then each store could subscribe to thestorage
event, and unsusbscribe during cleanup and we wouldn't need a global mapping of keys/stores.
No, that is unfortunately not possible. As described, storage event's only fire across context, and not in the same context, and while it would be possible to manually fire it, this would be incredibly messy, duplicate events (and related handling) and break standards. I think you made the right call with the caching structure, and as far as I can see, that isn't what's causing the problem in this specific issue
I don't know if you have heard about it, nor if it will be useful for you, but instead of listening to the storage
event, maybe the Broadcast Channel API can be helpful.
@PeppeL-G, that's a great point!
If we want to keep cross-tabs working, we could always roll our own channel events.
And then the storage
event could be captured
I've encountered a bug when two different pages are open at the same time and uses the same store/key.
Reproducible instructions:
git clone https://github.com/PeppeL-G/svelte-persisted-bug.git
cd svelte-persisted-bug
npm install
/
in a tab in the web browser (store1
is created)/page-2/show
in another tab in the web browser (store2
is created)/page-2/show
page, click on theHide counter
link (thestore2
variable is deleted)/
page, click on the inc button (store1
is properly incremented by 1)/page-2
page, click on theShow counter
link.store2
is created/retrieved from your internal cache, but it does not contain the incremented number instore1
even though they use the same keySo, in one and the same document, it seems like creating a store with the same key a second time does not use the latest value from local storage.