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

Summary StoreRecord.descendants/allDescendants return [undefined] #3721

Closed
amcclain opened this issue Jul 10, 2024 · 2 comments · Fixed by #3728
Closed

Summary StoreRecord.descendants/allDescendants return [undefined] #3721

amcclain opened this issue Jul 10, 2024 · 2 comments · Fixed by #3728

Comments

@amcclain
Copy link
Member

amcclain commented Jul 10, 2024

Due to impl of getDescendantsById() across Store and RecordSet - for a summary record, Store still delegates to RS to collect all descendants (including the record itself), but summary rec is not in RS, so its own ID lookup fails, returns undefined, and is not filtered out.

We should be returning [] from these getters - unless there's any expectation that summary records can carry children?

Considered simply early-out in the StoreRecord getters if isSummary but we should probably ensure the public Store.getDescendantsById() API also respects.

Could compact return from RecordSet.getDescendantsById() or iterate and conditionally push onto return. We should ensure whatever approach we take if performant.

Not a big bug but it did bite me in a master/detail view - was not expecting that undefined member.

@amcclain amcclain changed the title Summary StoreRecord.descendants and allDescendants return [undefined] Summary StoreRecord.descendants/allDescendants return [undefined] Jul 10, 2024
@lbwexler
Copy link
Member

So really weird to me that the public methods on Store and Record return the record itself! That's not documented, and seems totally inconsistent with getAncestorsById and getChildrenById. Seems to me we ought to fix that (carefully).

At the very least document it, but given the incongruity of it I would argue for the "breaking" change. I wonder if this method gets a lot of public use

@lbwexler lbwexler linked a pull request Jul 21, 2024 that will close this issue
6 tasks
@amcclain
Copy link
Member Author

As commented on the PR - agree, might be a change that breaks some apps relying on this behavior, but really does seem like a fix and the right one at that. (I am not one of my own descendants.)

I acknowledge it's a bit much, but we could introduce selfAndDescendants / selfAndAllDescendants getters for convenience - [this, ...this.descendants]. That would provide a 1-1 replacement for the prior incorrect impl for an app that wanted it, and it does seem like a potentially common pattern w.r.t. record selection - i.e. "I want this one and everything underneath it".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants