-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat(signals): add entities subpackage #4090
Conversation
✅ Deploy Preview for ngrx-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice Marko! 🏆
|
||
export type EntityState<Entity> = { | ||
entityMap: EntityMap<Entity>; | ||
ids: EntityId[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would mean that we support id arrays that has numbers and strings.
Do we want this?
I think it would be better to only allow one kind:
export type EntityId = string | number;
const ids: EntityId[] = [1, '2', 3];
// ^ OK
const idsOther: (string[] | number[]) = [1, '2', 3];
// ^ Type '(string | number)[]' is not assignable to type 'string[] | number[]'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review Tim! 🙏
Yes, I was aware of this behavior. I was wondering if it's worth defining a different type for EntityIds
as you mentioned because both string
and number
can be used to get the record property value by key.
For example, this code will compile:
const record: Record<number, number> = {
'1': 1,
};
although we specified that record keys can be only numbers, because in JavaScript this works:
const record = { 1: 1 };
console.log(record[1]); // logs 1
console.log(record['1']); // logs 1
Let me know if you think this should be changed and I'll add the EntityIds
type:
type EntityIds = string[] | number[];
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with both options, I added this comment just to be sure that it was intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think allowing strings as record keys should be supported for use cases such as uuids.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Closes #3996
What is the new behavior?
addEntity
addEntities
updateEntity
updateEntities
updateAllEntities
setEntity
setEntities
setAllEntities
removeEntity
removeEntities
removeAllEntities
Does this PR introduce a breaking change?