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

Provide setMany adapter for @ngrx/entity #3026

Closed
samuelfernandez opened this issue May 20, 2021 · 4 comments · Fixed by #3029
Closed

Provide setMany adapter for @ngrx/entity #3026

samuelfernandez opened this issue May 20, 2021 · 4 comments · Fixed by #3029
Labels
Accepting PRs community watch Someone from the community is working this issue/PR enhancement Project: Entity

Comments

@samuelfernandez
Copy link
Contributor

samuelfernandez commented May 20, 2021

setAll replaces the whole collection. If an entity is not provided there, it will be removed. However, the feature request for a new setMany would act like:

  • Replacing (not updating) the elements being provided, like setOne
  • Keep other entities that are not updated

Copying from the docs:

To prevent partial updates either explicitly set all the fields, setting non-used fields with value undefined, or use the setOne or setAll adapter methods.

Even though there is updateMany/upsertMany, those do a partial update. The requested setMany would do a replacement.

Describe any alternatives/workarounds you're currently using

Custom entity adapter extension or a loop on setOne.

Other information:

We have a scenario where we want to set only a subset of entities, so setAll wouldn't work. This would align with remove, update and upsert.

If accepted, I would be willing to submit a PR for this feature

[x] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

@timdeschryver
Copy link
Member

Hey, we already have a setAll method (https://ngrx.io/guide/entity/adapter#adapter-collection-methods).
In earlier versions of NgRx, this method was be named addAll.

@samuelfernandez
Copy link
Contributor Author

samuelfernandez commented May 20, 2021

@timdeschryver setAll would replace the whole collection. If an entity is not provided there, it will be removed. However, the feature request for setMany would act like:

  • Replacing (not updating) the elements being provided, like setOne
  • Keep other entities that are not updated

Would you reconsider the proposal? Even though there is updateMany/upsertMany, those do a partial update. The requested setMany would do a replacement.

Copying from the docs:

To prevent partial updates either explicitly set all the fields, setting non-used fields with value undefined, or use the setOne or setAll adapter methods.

So the functionality of setMany is not there. Sorry that the feature request was not so clear, I've update the description. Would you consider re-opening?

@timdeschryver
Copy link
Member

timdeschryver commented May 21, 2021

Now, that's a whole different story 😅
I think that would complete the entity api, as we have a xxOne and xxMany for the other methods.
Feel free to create Pull Request if you'd want to.

@samuelfernandez
Copy link
Contributor Author

@timdeschryver thanks for allowing this move forward! 😄 I've submitted a PR, please let me know if there is anything left

@timdeschryver timdeschryver added Accepting PRs community watch Someone from the community is working this issue/PR labels May 22, 2021
brandonroberts pushed a commit that referenced this issue May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepting PRs community watch Someone from the community is working this issue/PR enhancement Project: Entity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants