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

feat: indexedDB support #271

Closed
wants to merge 3 commits into from
Closed

Conversation

intercepted16
Copy link
Contributor

Implemented indexedDB support for the persisted store. This allows data to persist in indexedDB instead of sessionStorage or localStorage, useful for larger datasets.

Key changes include:

  • Localforage is now used for a consistent API
  • All operations are now asynchronous due to localforage being asynchronous
  • As localforage does not support sessionStorage, the default sessionStorage is used (no problems are caused)

More specific changes are listed in each commit.

PS: All operations are asynchronous, and a type warning is not provided as the methods of the persisted store do not return.

@intercepted16 intercepted16 force-pushed the master branch 2 times, most recently from ad18a8f to a53cde9 Compare June 2, 2024 06:52
- Modify StorageType to include IndexedDB

- Use localforage for localStorage and IndexedDB operations

- All functions are asynchronous due to this

- A mock IndexedDB is provided in test and development mode

PS: This change makes all storage operations asynchronous, which may require updates in parts of the codebase that use these operations.
- Rename existing storage type test to be more specific

- Add indexedDB storage type check
@joshnuss
Copy link
Owner

joshnuss commented Jun 3, 2024

Thanks @intercepted16!

I'm not totally sure, as it it contains breaking changes. It would mean all user's of this package would need to upgrade, even if they don't use Indexed DB.

For now, I don't think it's a good time to add it. Maybe it can be considered as part of the change to Runes API. (since that will require breaking changes anyways)

P.S. Would prefer if larger changes start as discussions, because I feel bad to turn away someone's effort

@joshnuss joshnuss closed this Jun 3, 2024
@joshnuss joshnuss added the wontfix This will not be worked on label Jun 3, 2024
@webJose
Copy link

webJose commented Jun 3, 2024

My 2 cents: This brings an extra package. This increases bundle sizes for people that don't used indexed DB. I would vote against this even for the runes version. Perhaps it could be created as a separate package. I don't think there's enough justification to force asynchronous API's that 90+% of people won't need.

@intercepted16
Copy link
Contributor Author

@webJose I would just like to add that localforage is actually quite lightweight; it is only 8.8kb when gzipped. However, I do agree with the forced asynchronous API's.

@webJose
Copy link

webJose commented Jun 5, 2024

The less packages your application uses, the better in terms of maintenance. Packages become obsolete, so yoiu have to keep on updating things. Packages might also provide a surface for attacks. If you bring a package to your application that you don't need, you might end up getting extra work to clean up a CVD about it. What for? Nothing. Your app is not needing it. You are just doomed to maintain it because it comes with another package that you do use. It is good to apply the Single Responsibility Principle to packages.

@intercepted16
Copy link
Contributor Author

intercepted16 commented Jun 6, 2024

@joshnuss If I modify it to use a synchronous API without localforage or an external package, would the PR be accepted?

@joshnuss
Copy link
Owner

joshnuss commented Jun 6, 2024

@intercepted16 My understanding is that the IndexDB API is inherently asynchronous.

An alternative approach is having different function for each storage method.

// old-style of import would be deprecated
import { persisted } from 'svelte-persisted-store'

// instead there would be separate functions for localStorage, sessionStorage and IndexDB
import { localState, sessionState, indexDBState } from 'svelte-persisted-store'

Then the IndexDB version would have slightly different signature (async) and slightly different implementation (no need for serialization with indexdb)

@intercepted16
Copy link
Contributor Author

@joshnuss Sorry, I made an error in my comment. I will try to implement your suggestion.

@joshnuss
Copy link
Owner

joshnuss commented Jun 6, 2024

Before you implement, let me ask for feedback from a few folks

@bertmad3400 @webJose @niemyjski any thoughts on this proposed change?

@webJose
Copy link

webJose commented Jun 6, 2024

Hello. Strictly speaking, if the feature can be tree-shaken, theoretically it would not affect bundle sizes. Using different functions as @joshnuss suggested would be the right move in the tree-shakable direction.

Still, the feature is so unique that I don't think it should be covered by this package, at least not as it is today. I don't think many people need to store a lot of data in the browser. This is, in my opinion, an edge-case feature.

A bold suggestion would be to break everything and re-think this package thinking plug-ins: Make this package provide the reactive parts, but don't provide storage of any kind. Instead, users of the package would have to install another package that would add the storage part. Therefore, users will be able to use the same "front" logic of this package with any storage they like.

Examples:

  1. WebJose needs session storage. WebJose therefore installs svelte-persisted-store and svelte-persisted-store-session.
  2. Joshnuss needs indexedDB storage. Joshnuss therefore installs svete-persisted-store and svelte-persisted-store-indexedDB.

Etc. A system where one package provides reactivity (the Svelte part), and another package provides the storage. Too radical? Perhaps. Still, strictly speaking such system satisfies my requirements of not increasing bundle sizes and not changing the interface.

On the other hand, indexedDB has asynchronous API. Forcing the API to synchronous is something I don't like. In the plug-in system I would then standardize all storage API to be aysnchronous. This is the downside I see.

So, practical terms: Joshnuss' suggestion seems to be the more likely one for me.

@bertmad3400
Copy link
Contributor

So I have a couple of thoughts:

  • This project really needs a formatter. Shouldn't be a big change, but makes diffs like these much, much more readable.
  • Personally I could definitely see the use of an indexedDB option. The storage limits on localStorage means that there are some cases where that isn't viable.
  • The breaking API change, by requiring everything to be async would for me (had I been the library maintainer) be a deal breaker as well. For this problem I could see two potential solutions:
  1. Implement this as an option param (so besides localStorage and session, the storage type could be indexedDB) along with some complex types, which would modify return types of the store based on the set storage type option param. This has the advantage of making this minimally invasive to the user, but would probably massively complicate the codebase.
  2. Implement this as a second store. I think this is the most viable option, though it would probably require some refactoring work of the existing code-base.
  • If we are to include indexedb as an option here (and I really think we should consider that), I would love to see a draft of implementing the required interaction with indexedDB natively. I too don't like the idea of relying on localforage for this functionality, and I have a hunch that while indexedDB surely is more complicated than the localStorage API, I wouldn't believe it would be too terribly difficult.

@intercepted16
Copy link
Contributor Author

@webJose @bertmad3400 I have implemented @joshnuss's solution of three functions; localState, indexedDBState and sessionState in a new pull request: #273

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants