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

@ngrx/data pr #3228 broke the loaded flag without introducing an alternative #4025

Open
1 of 2 tasks
VFiber opened this issue Sep 1, 2023 · 2 comments
Open
1 of 2 tasks

Comments

@VFiber
Copy link

VFiber commented Sep 1, 2023

Which @ngrx/* package(s) are the source of the bug?

data

Minimal reproduction of the bug/regression with instructions

Before #3228 @ngrx/data provided an option to query if the local Entity-cache already contains a snapshot of every entity trough the ˙loaded flag.

Although the naming was a bit off, after #3228 there is no option to determine if every entity is loaded or if the store only contains partial data.

What makes this as a bug: Since V8 (ngrx/data introduced) this property was not changed in the documentation (see: https://v8.ngrx.io/guide/data/entity-collection, https://ngrx.io/guide/data/entity-collection (v16)).

Expected behavior

loaded flag only gets true when QueryAll successfully executed and the data patched back to the state.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)

NgRx: 13.0

Other information

My suggested alternative solution is:

  • add a new loadedType: partial / full / not_loaded flag and fill it / introduce a simple boolean allLoaded as smorandi suggested in getWithQuery method should set loaded property to true #3165 in the original issue comments
  • customize the behavior in EntityDataModuleConfig and create an option to revert to the old behavior for everyone who upgrades from v12 does not have to re-write the whole cache-logic, but make it deprecated to indicate this is not the intended use
  • fix the documentation, clearly state what loaded flag means in the new context, reflect on the fact that the behaviur was changed in v13
  • include the alternative solutions in the docs that replaces / restores the previous functionality and a deprecation notice as it will be removed in the next 1-2-3 version.

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@VFiber
Copy link
Author

VFiber commented Sep 1, 2023

After trying to work around the change found some additional logical issues with it.

If it is a flag for successful load why it does affect QUERY_MANY_SUCCESS, and not QUERY_BY_KEY_SUCCESS?

With the new version in mind, I would assume it is a flag and applies to every QUERY_*_SUCCESS Action when data has been successfully retrieved from the API.

@martijnMedialen
Copy link

please fix this!

Maybe also add an option to set loaded state because i have a usecases where I want to load a subset of my data (only loading active users and want to have an option to load all later) and have the previous workaround build in.

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

No branches or pull requests

2 participants