Skip to content

feat(with-storage-sync): indexeddb support #134

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

Merged

Conversation

mzkmnk
Copy link
Contributor

@mzkmnk mzkmnk commented Feb 2, 2025

Issue

#111

Content

In this PR, we modified withStorageSync to support IndexedDB.

Breaking Changes

  • The storage argument has been removed, and a new second argument now allows you to choose withIndexeddb(), withLocalStorage(), or withSessionStorage(). By default, withLocalStorage() is used.

Sample Code

const TodoIndexedDBSync = signalStore(
  withState<...>({...}),
  withStorageSync(
    {key:'todos'},
    withIndexeddb(),
  ),
  withMethods((store) => ({...}))
);

@mzkmnk mzkmnk marked this pull request as ready for review February 2, 2025 09:46
@mzkmnk
Copy link
Contributor Author

mzkmnk commented Feb 2, 2025

@rainerhahnekamp
The code is complete. Please review it! :octocat:

@mzkmnk
Copy link
Contributor Author

mzkmnk commented Feb 6, 2025

@audhit
What do you think of this fix?

@audhit
Copy link

audhit commented Feb 7, 2025

@audhit What do you think of this fix?

Hey @mzkmnk looks promising. This is exactly what I am looking for. NGRX tool will be complete with this feature to be honest. @rainerhahnekamp please verify and merge it soon if possible and let us know about the new release. Thanks a lot for the great work guys 🙏

@mzkmnk mzkmnk force-pushed the feat/add-dexie-support branch 2 times, most recently from 1fb0d88 to 843c38f Compare February 8, 2025 12:21
@rainerhahnekamp rainerhahnekamp self-assigned this Feb 9, 2025
@rainerhahnekamp rainerhahnekamp added the enhancement New feature or request label Feb 9, 2025
Copy link
Collaborator

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

Very good work!

I think my biggest suggestion would be this refactoring to withIndexedDB and withLocalStorage().

Other than that, please make sure that the Git History is kept intact (I've added a comment about it).

If there is something unclear, please let me know. I have now a good overview and should be able to respond promptly.

@rainerhahnekamp
Copy link
Collaborator

@mzkmnk an additional explanation, via it should be withIndexedDB() and not it is right now.

With the current implementation, we have the IndexedDB service in the bundle, even if it is not used. If that Service only is imported via withIndexedDB() and no one is calling/importing withIndexedDB(), it never ends up in the bundle.

BREAKING CHANGE: The storage argument has been renamed to storageType, and its value must now
be one of the following strings: 'localStorage', 'sessionStorage', or 'IndexedDB'.
Additionally, when 'IndexedDB' is specified, two new arguments, `storeName` and `dbName`,
have been added.
@mzkmnk mzkmnk force-pushed the feat/add-dexie-support branch from f73afeb to 8189b66 Compare February 15, 2025 02:54
@mzkmnk
Copy link
Contributor Author

mzkmnk commented Feb 15, 2025

@rainerhahnekamp
I’ve finished making the changes. Please review them!

@mzkmnk
Copy link
Contributor Author

mzkmnk commented Feb 19, 2025

@rainerhahnekamp

Could you explain this a little bit? Seems I am lacking some knowledge in the way how indexeddb works.

Regarding this topic, I’ve created a demo here, so please give it a try:
https://stackblitz.com/edit/stackblitz-starters-hjuxek6r?file=src%2Findexeddb.service.ts

The cause of the error is that, in IndexedDB, you have to increase the database version when creating a new object store in an existing database. Because of this, if the library were to fix the version internally, we would need to handle version management ourselves, which would make things more complex. For that reason, I’m currently using the same name for both the storeName and the dbName.

I’ve found a better approach than the current one, so I’d like to share it. By fixing the dbName and storeName, and using the IndexedDB keyPath as the key, multiple pieces of data can be stored in a single database. I’ve rebuilt the implementation using this method:
https://stackblitz.com/edit/stackblitz-starters-eb4mwctq?file=src%2Fmain.ts

@audhit
Copy link

audhit commented Feb 21, 2025

@rainerhahnekamp looks good to me, can you please kindly test @mzkmnk's solution and let us know if you can merge and release it? It would be a great help in my project. Thanks in advance 🙏

@rainerhahnekamp
Copy link
Collaborator

@audhit, we are in the middle of the second set of change requests. We can't merge now.

@rainerhahnekamp
Copy link
Collaborator

@mzkmnk

using the IndexedDB keyPath as the key, multiple pieces of data can be stored in a single database

So we are saying we are using the same dbName and dbName for all instances, right? If yes, please implement that 👍

@mzkmnk
Copy link
Contributor Author

mzkmnk commented Feb 21, 2025

@rainerhahnekamp

So we are saying we are using the same dbName and dbName for all instances,

This one has already been corrected.Please check.

55e81df

@audhit
Copy link

audhit commented Mar 3, 2025

@audhit, we are in the middle of the second set of change requests. We can't merge now.

Waiting for it eagerly 🚀 , please kindly make it happen soon if possible 🚀 @rainerhahnekamp .... thanks a lot in advance

Copy link
Collaborator

@rainerhahnekamp rainerhahnekamp left a comment

Choose a reason for hiding this comment

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

@mzkmnk I think we should get that issue with git history sorted out and then merge it.

As I said in a comment before. Maybe it is better to move the with-storage-sync.(spec).ts files back where they were and we'll try to do the "git mv" in a separate branch.

@mzkmnk
Copy link
Contributor Author

mzkmnk commented Mar 4, 2025

@rainerhahnekamp

I understand. I will move with-storage-sync.(spec).ts back to its original location. Also, regarding c041a49, since it's not really a good fix, would it be okay to revert the changes?

@rainerhahnekamp
Copy link
Collaborator

@mzkmnk

I understand. I will move with-storage-sync.(spec).ts back to its original location. Also, regarding c041a49, since it's not really a good fix, would it be okay to revert the changes?

I would leave everything as it is, so no reverting. The only thing is really about keeping the git history intact. That should do it.

@mzkmnk
Copy link
Contributor Author

mzkmnk commented Mar 4, 2025

@rainerhahnekamp

I've completed moving the with-storage-sync.(spec).ts file.
c45b665

@rainerhahnekamp
Copy link
Collaborator

Wonderful @mzkmnk. I'm going to squash and merge it. Congratulations!

@rainerhahnekamp rainerhahnekamp merged commit fd3ea12 into angular-architects:main Mar 4, 2025
1 check passed
@rainerhahnekamp
Copy link
Collaborator

@mzkmnk do you want to create the PR which just moves with-storage-sync.*.ts files or should I do that?

@mzkmnk
Copy link
Contributor Author

mzkmnk commented Mar 4, 2025

@rainerhahnekamp

After this PR is merged, I’ll create a PR to move the with-storage-sync.(spec).ts files.

@mzkmnk mzkmnk deleted the feat/add-dexie-support branch March 5, 2025 06:49
@audhit
Copy link

audhit commented Mar 6, 2025

wowowow...great work guys...now new release and documentation update needs to be done...its soo great and good feeling when someone's request is taken care of. I am so thankful to you guys @mzkmnk and @rainerhahnekamp ... Thank you so much, waiting for it to be released...thanks again :)

@rainerhahnekamp
Copy link
Collaborator

Full kudos ot @mzkmnk

@mzkmnk
Copy link
Contributor Author

mzkmnk commented Mar 6, 2025

@rainerhahnekamp
@audhit
My first OSS development experience was a great memory. Thank you!

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

Successfully merging this pull request may close these issues.

3 participants