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

Jump to bottom

ChangeItemSetFactory Update does not allow entities with Number IDs #1988

Closed
allout58 opened this issue Jul 2, 2019 · 1 comment · Fixed by #1995
Closed

ChangeItemSetFactory Update does not allow entities with Number IDs #1988

allout58 opened this issue Jul 2, 2019 · 1 comment · Fixed by #1995
Labels
bug Project: Data

Comments

@allout58
Copy link
Contributor

allout58 commented Jul 2, 2019

Minimal reproduction of the bug/regression with instructions:

https://stackblitz.com/edit/ngrx-seed-data-update-bug

See the type error in the example() function, compared with an entity with a String ID

Expected behavior:

ChangeSetItemFactory.update should allow for updating entities with a Number type ID

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

NgRx: 8.0.1
Angular: 8.0.2
Node: 10.15.3

Other information:

Although they type Update<T> allows for UpdateStr<T> | UpdateNum<T>, the update function forces T to extend {id: string} only

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

[X] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No
@timdeschryver
Copy link
Member

The typings are wrong(?) at https://github.com/ngrx/platform/blob/master/modules/data/src/actions/entity-cache-change-set.ts#L88

allout58 added a commit to allout58/platform that referenced this issue Jul 3, 2019
@allout58
fix(data): allow ChangeSetItemFactory to update entities with number ids
797391d 
Allow the `update` method of ChangeSetItemFactory to accept entities with numerical IDs.
This seems to be the intent, as the `Update<T>` it accepts can use either a string or a number for the ID, while the `update` method only allows for string IDs

Closes ngrx#1988
@allout58 allout58 mentioned this issue Jul 3, 2019
Merged
3 tasks
brandonroberts pushed a commit that referenced this issue Jul 7, 2019
@allout58 @brandonroberts
fix(data): allow ChangeSetItemFactory to update entities with number …
f11c7b2 
…ids (#1995)

Allow the `update` method of ChangeSetItemFactory to accept entities with numerical IDs.
This seems to be the intent, as the `Update<T>` it accepts can use either a string or a number for the ID, while the `update` method only allows for string IDs

Closes #1988
jordanpowell88 pushed a commit to jordanpowell88/platform that referenced this issue Nov 14, 2019
@allout58 @jordanpowell88
fix(data): allow ChangeSetItemFactory to update entities with number …
acde23e 
…ids (ngrx#1995)

Allow the `update` method of ChangeSetItemFactory to accept entities with numerical IDs.
This seems to be the intent, as the `Update<T>` it accepts can use either a string or a number for the ID, while the `update` method only allows for string IDs

Closes ngrx#1988
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Project: Data
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(data): allow ChangeSetItemFactory to update entities with number ids allout58/platform
2 participants
@allout58 @timdeschryver