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 #273

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

intercepted16
Copy link
Contributor

@intercepted16 intercepted16 commented Jun 9, 2024

Implement IndexedDB support, useful for storing larger amounts of data.

  • The persisted function is now deprecated, but remains functional
  • Instead of a storage option, there are now three seperate functions;
    localState, sessionState and indexedDBState
  • The localState and sessionState functions remain the same, aside from the storage parameter
  • indexedDBState is asynchronous and uses async/await (or callback)
  • Some refactoring was done, specifically with types

PS: persisted remains functional with the same API

dependabot bot and others added 3 commits June 9, 2024 18:29
Bumps [rollup-plugin-svelte](https://github.com/sveltejs/rollup-plugin-svelte) from 7.1.0 to 7.2.0.
- [Changelog](https://github.com/sveltejs/rollup-plugin-svelte/blob/master/CHANGELOG.md)
- [Commits](sveltejs/rollup-plugin-svelte@v7.1.0...v7.2.0)

---
updated-dependencies:
- dependency-name: rollup-plugin-svelte
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
- The persisted function is now deprecated, but remains functional
- Instead of a storage option, there are now three seperate functions;
 localState, sessionState and indexedDBState
- The localState and sessionState functions remain the same, aside from the storage parameter
- indexedDBState is asynchronous and uses async/await (or callback)
- Some refactoring was done, specifically with types

PS: persisted() remains functional with the same API
Copy link

@webJose webJose left a comment

Choose a reason for hiding this comment

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

Generally speaking, and without reviewing every single change, I would say the change looks well done. Since the entries are separated functions, this should tree-shake well as far as I know. It would be ideal if tree-shaking tests could be added in order to verify that existing users won't get extra stuff in their bundles unnecessarily.

@joshnuss, do you have a roadmap as to when to get out of experimental versions? I see you have kept the versions experimental for a long time. What is the requirement to hit v1.0.0? I ask because this PR looks like a good candidate to hit it. Maybe you could also give this a round of release-candidate versions before becoming final (1.0.0-rc.1)? If you agree, maybe it is also time to get rid of writable()?

types/options.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
@webJose
Copy link

webJose commented Jun 10, 2024

@intercepted16 thanks for the udpated PR, but please, don't continue unless @joshnuss says so. I just expressed my opinion in the form of a PR review. That's it. It is unclear to me if @joshnuss is even agreeable to adding this feature.

@intercepted16
Copy link
Contributor Author

@webJose Your changes were small, so I figured implementing them wouldn't hurt.

@intercepted16
Copy link
Contributor Author

@joshnuss Are you willing to add this feature? It's been a while with no response.

@joshnuss
Copy link
Owner

Hi, I do t have a good answer right now.

The next priority for the project is adding support for runes, so I'm not sure we should add more features right now.

Is this an important requirement you're waiting on?

@intercepted16
Copy link
Contributor Author

@joshnuss Yes, I'd prefer to use the library for this; currently I'm using a custom solution. Maybe, as breaking changes are being implemented anyway, you could include this pull request in the runes change?

@plutonium-239
Copy link

I second (third) this PR/feature!

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.

4 participants