fix: storage syncing for feature stores

[Abhirocks889]

Summary

This PR introduces a fix for the issue not syncing with feature stores

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines:
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• Other... Please describe:

Current Behavior

Currently, it is not possible to sync the state of a feature store by adding the meta reducer inside the feature module. Instead, it has to be done within the app module for all the feature stores.

Issue Number: 96

New Behavior

With this fix, it is now possible to sync the state of any store directly within the feature module in addition to being able to perform the same from within the app module.

Breaking Change?

• Yes
• No

Other information

As per my understanding and observation, the basic difference between syncing a state in the root app module via StoreModule.forRoot versus in a feature through StoreModule.forFeature is the fact that in the former, the syncStateUpdate method receives the entire State where each feature is described by key-value pairs (eg: {cart: {items: []} checkout: {methods: []}}), whereas in the latter, the syncStateUpdate method receives the respective feature slice state (eg: {items: []} for cart and {methods: []} for checkout).

PFA the screenshots illustrating the same:

Syncing in the app module

Syncing in the feature module

Therefore, the latter scenario was not handled as it always looked for the key specified in the config and fetch the corresponding state for the key.

The fix introduces a new boolean property to the LocalStorageConfig called forFeature which when enabled, handles the state syncing for feature stores.

Note: PFA a stackblitz that illustrates the behavior after the fix: https://stackblitz.com/edit/angular-ivy-bdu1sx?file=README.md

[Abhirocks889]

@BBlackwo Could you please review the changes whenever you find some time. Thanks

[tinesoft]

Any plan on reviewing/merging that PR soon @BBlackwo ?
It would be a very welcome addition/fix to this great library !

[faileon]

Can we merge this please? I would love to use it also instead of defining it in the root store.

[inthegarage]

Can we merge this please? I would love to use it also instead of defining it in the root store.

+1

Also defining it in the root store means I must create maps and the like to hold the reducers, = more code. Also really appreciate this going in.

[BBlackwo]

Hey thanks for the PR @Abhirocks889! Given it's a fairly significant change I'm not sure when I'll get a chance to review it properly. I'm maintaining this project in my spare time, doing my best.

[inthegarage]

@BBlackwo Is there anything I can do to lend a hand? I did have a nosey at the PR and it did seem a large change. If I can do some of the lifting, then happy to help. I use this library daily and need this feature.

[BBlackwo]

If some of you that are interested can test it out in your projects and give feedback that'd be helpful. You should be able to use the branch directly in your package.json:

"dependencies": {
  "ngrx-store-localstorage": "Abhirocks889/ngrx-store-localstorage#fix\/feature-stores"
}

[BBlackwo]

@inthegarage see the above comment about testing the branch.

Also happy for anyone to review the PR and leave comments. You don't have to be a maintainer to review the PRs 🙂

[Abhirocks889]

Hello @BBlackwo , I agree that it is maybe a significant change but a non-breaking one as it is controlled by the flag in the StorageConfig. Having said that, it would be great for others to try out the change and provide feedback.

[inthegarage]

@BBlackwo At the moment I could not get the feature to work, I'll have a another go later. From a code point of view it does seem fine, although I'd like to see the use of the boolean changed, so that I don't have to keep saying "true" all the time. I'll make a formal comment once I have tested it properly. The linting and tests all pass though, so that's good.

[inthegarage]

@Abhirocks889 I'm trying to test your PR, however when I do a build_dist I get the following error:

Could not resolve "./lib/index" from "dist/lib/esm2020/public_api.mjs"

This is when I do npm run build_dist

Everything node_modules wise installed correctly. Do you know the correct build sequence?

[dofamine]

any updates on this bug?